-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Teal to PyTeal Source Mapper #650
Conversation
Co-authored-by: Zeph Grunschlag <[email protected]>
Merge `feature/fp-router` into `bug-router-not-idempotent`
Co-authored-by: Hang Su <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is roughly in shape, let me revert back for a few following things:
- Teal to PyTeal Source Mapper #650 (comment) (usage of deprecated method about subroutine declaration)
- Teal to PyTeal Source Mapper #650 (comment) (similar usage of deprecated method about subroutine declaration)
- working through a unit test #682 (compilation disagreement)
Consider this as an informal approval once we figure out how to resolve the problems above.
Thanks for the informal approval. I agree that the interrelated issues pointed to above are a blocker for releasing this PR. |
Since #684 is merged in, we can resolve the merge conflict at some point, and hopefully we will get this one merged too. |
Co-authored-by: Hang Su <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Source Mapper for PyTeal
Notes for PR Reviewers
The most important changes to gain understanding for the workings of the source mapper are in:
pyteal/ast/router.py
pyteal/compiler/compiler.py
pyteal/compiler/sourcemap.py
pyteal/stack_frame.py
I won't makeI'm trying to minimize major changes to those files until I receive review comments. However, there may be small changes that result from cleaning up other files or merging in changes to upstream branches. I'll make sure to add a new comment and tag the reviewers every time such a change occurs.FAQ
What do I need to do in order to make source mapping available in PyTeal ?
NO LONGER CORRECT AND OUT OF DATE
You'll need to turn it on in a
pyteal.ini
file that will live at the top level of your python repo.Moreover, the
pyteal.ini
should look like:What if as a consumer of PyTeal, I don't supply a
pyteal.ini
?NO LONGER CORRECT AND OUT OF DATE
Everything should continue to work. HOWEVER, some error messages will be printed out. For example, when I experimented running AlgoKit I got the following printouts, before I added a
pyteal.ini
:What if I want to map program counters (PC's) as well?
You'll need to ensure that a dev-mode-enabled algod is running on port 4001 (for example, this is the Sandbox's default port).
How do I get source mapping info using the
Router
?class Router
has a newcompile()
method which returns aRouterResults
object. (The API of the originalcompile_program()
method remains the same). For example, usage might look like:What if I want the source mapping without using the
Router
?The API for
pyteal.compiler.compileTeal()
hasn't changed, so it doesn't provide a source map.However, there is a new class
pyteal.compiler.Compilation
which has its owncompile()
method(returning a
CompilerResults
object) that can produce source maps.Here's an example usage. Note that the
PyTealSourceMap
type is the same as above.(by @bbroder-algo ) Do you know why
sourcemap_monkey_unit_test::test_constructs
is so much slower on 3.10?I’m not sure as I haven’t run a profiler in a way that would answer this question, but I suspect it has to do with Streamlined Stack Frames
Does the Source Mapper work with AlgoKit and Beaker?
NO LONGER CORRECT AND OUT OF DATE
Yes, no, and maybe.
pyteal.ini
I was able to get source mapping to work with AlgoKit. Here's an example repo: https://github.com/tzaffi/ak-source-mapper. The bottom of the README.md provides some hints to get it all working.What do the Source Mapper artifacts look like in AlgoKit?
NO LONGER CORRECT AND OUT OF DATE
Here's an example from a recent run on AlgoKit's HelloWorld example: https://github.com/tzaffi/ak-source-mapper/tree/main/smart_contracts/artifacts/helloworld.HelloWorld
What assurances do we have that the source mapper introduces no changes to the actual compiled TEAL ?
Functionally we can see near compiler.py L465 that source mapping begins only after the compilation process has ended and produced the
teal_code
artifact (L453). However, there are many small changes to areas of the compiler's logic, that if done carelessly, would introduce a bug.There are validation to ensure the source-mapped teal is identical to the teal produced at the same time
Look for the following
_PyTealSourceMapper
methods in the codebase:_validate_build()
_validate_annotated()
_validate_teal_identical()
We also have the following tests to guard against regressions:
sourcemap_monkey_unit_test.py
pure_teal()
method providessourcemap_test.py
use the AlgoBank examples under various configurations to guard against regressions:router.compile_program()
with the sourcemapping as is currently configured in the repoTODONE (former TODO's)
test_sourcemap_annotate()
are compatible with the pre-existing fixtures.router.py
compiler.py
sourcemap.py
stack_frame.py
sourcemap_monkey_integ_test.py
test_constructs
unit testsmaster
branchmake nightly-slow
command for the very slow testspyteal.ini
, UNLESS they're actually trying to run a source maptests/unit/sourcemap_monkey_unit_test.py::test_r3sourcemap
- currently skipping 3.10tests/unit/sourcemap_monkey_unit_test.py::test_constructs
- extremely slow on 3.10sourcemap_monkey_unit_test.py::test_constructs[8-Application-37-test_case37]
(and 38) failing due to upcoming changes in routerSplit out more unrelated changes???pyteal/ast/subroutine.py
typingpyteal/ir/__init__.py
CHANGELOG.md
tests/unit/sourcemap_rps_test.py
TODO (before merge)
All done for now!
TODO (create issues)
StackFrame
to no longer rely onFrameInfo
's and therefore be a lot faster (cf. https://github.com/jasonpaulos/pyteal/tree/source-map-performance)FrameInfo
is complete we ought to:enabled = False
in thepyteal-source-mapper
section)NatalStackFrame
to provide a single frame instead of a listStackFrame - PyTealFrame - TealMapItem
hierarchymjpieters
's SourceMap to get faster R3SourceMap (cf: https://github.com/tzaffi/pyteal/blob/f2a945a8eeb396e9c544b62cd3d1a6383bfc9bff/pyteal/compiler/sourcemap.py#L75)._sframes_container
are actually necessary. To see all usages: f96ef86_PyTealSourceMapper
(cf: https://github.com/tzaffi/pyteal/blob/f2a945a8eeb396e9c544b62cd3d1a6383bfc9bff/pyteal/compiler/sourcemap.py#L730)_PyTealMapper.annotate()
(cf: https://github.com/tzaffi/pyteal/blob/f2a945a8eeb396e9c544b62cd3d1a6383bfc9bff/pyteal/compiler/sourcemap.py#L844)*.ini
andpyproject.toml
files. In particular, in the case thatpyteal.ini
isn't found, look also forpyproject.toml
, and then for the section[tool.pyteal.ini]
Such a section should look like: