Skip to content
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

Merged
merged 379 commits into from
Mar 7, 2023
Merged

Teal to PyTeal Source Mapper #650

merged 379 commits into from
Mar 7, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Jan 26, 2023

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 make I'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, thepyteal.ini should look like:

[pyteal]

[pyteal-source-mapper]
enabled = True
debug = False

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:

❯  ... -- -m smart_contracts
Turning off frame capture and disabling sourcemaps. 
Could not read section (pyteal-source-mapper, enabled) of config "pyteal.ini": No section: 'pyteal-source-mapper'
Disabling `debug` status for sourcemaps. 
Could not read section (pyteal-source-mapper, debug) of config "pyteal.ini": No section: 'pyteal-source-mapper'

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 new compile() method which returns a RouterResults object. (The API of the original compile_program() method remains the same). For example, usage might look like:

router = Router(name="GreatDapp")

@router.method
def add(a: abi.Uint64, b: abi.Uint64, *, output: abi.Uint64) -> Expr:
    return output.set(a.get() + b.get())

results: RouterResults = router.compile(
    version=8,
    with_sourcemaps=True,
    pcs_in_sourcemap=True,
    annotate_teal=True,
    annotate_teal_headers=True,
    annotate_teal_concise=True,
)

# because `with_sourcemaps == pcs_in_sourcemap == annotate_teal == True`
# `results` is "full" and the following assertions will hold

assert results.approval_sourcemap                 # of type PyTealSourceMap
assert results.clear_sourcemap                    # of type PyTealSourceMap

assert results.approval_sourcemap.r3_sourcemap    # of type R3SourceMap
assert results.approval_sourcemap.pc_sourcemap    # of type PCSourceMap
assert results.approval_sourcemap.annotated_teal  # of type str
assert results.clear_sourcemap.r3_sourcemap       # of type R3SourceMap
assert results.clear_sourcemap.pc_sourcemap       # of type PCSourceMap
assert results.clear_sourcemap.annotated_teal     # of type str

# because `annotate_teal_headers == pcs_in_sourcemap == True`
# the annotated teal will include the PC information. I.e., it may look something like the following:

print(results.sourcemap.annotated_teal)
// GENERATED TEAL                      //    PC     PYTEAL PATH                      LINE   PYTEAL
#pragma version 8                      //    (0)    whatever/path/to/this/script.py  7      router.compile(version=8, ...)
txn NumAppArgs                         //    (20)
int 0                                  //    (22)
...

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 own compile() 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.

comp: Compilation = Compilation(Int(42), mode.Application, version=8)
results: CompilerResults = comp.compile(
    with_sourcemap=True,
    pcs_in_sourcemap=True, 
    annotate_teal=True,
    annotate_teal_headers=True,
    annotate_teal_concise=True,
)

# because we chose the parameters above, the following assertions will hold:

assert results.sourcemap  # of type PyTealSourceMap
assert results.sourcemap.r3_sourcemap
assert results.sourcemap.pc_sourcemap
assert results.sourcemap.annotated_teal

(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.

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:

TODONE (former TODO's)

  • Add a test to sourcemap_monkey_integ_test.py which asserts that all the fixtures for test_sourcemap_annotate() are compatible with the pre-existing fixtures.
  • Turn tests back on that were off due to router's non-idempotency
  • ASAP Refactors
    • router.py
    • compiler.py
    • sourcemap.py
    • stack_frame.py
    • sourcemap_monkey_integ_test.py
  • Comments describing what's actually happening in detail
  • Not skipping any of the test_constructs unit tests
  • New nightly tests
    • Should only run if there have been changes to within the previous 24 hours
    • Need to be tested on master branch
    • Carve out a new make nightly-slow command for the very slow tests
  • Don't print out ugly error messages just because someone doesn't have a pyteal.ini, UNLESS they're actually trying to run a source map
  • Decide if to turn back on python 3.10 skipped tests (they'll need their own test cases)
    • tests/unit/sourcemap_monkey_unit_test.py::test_r3sourcemap - currently skipping 3.10
    • tests/unit/sourcemap_monkey_unit_test.py::test_constructs - extremely slow on 3.10
    • sourcemap_monkey_unit_test.py::test_constructs[8-Application-37-test_case37] (and 38) failing due to upcoming changes in router
  • NOT DOING Split out more unrelated changes???
    • pyteal/ast/subroutine.py typing
    • pyteal/ir/__init__.py
  • Abort and refuse to provide source mapper when it disagrees with the un-mapped teal
  • Resolve Source Mapper: Poor Mapping of Subroutines #658
  • Update CHANGELOG.md
  • (b5daedd) Merge in DO NOT MERGE: RPS fixture test to assert no regressions #669 to ensure that fixtures are identical to those used by tests/unit/sourcemap_rps_test.py

TODO (before merge)

All done for now!

TODO (create issues)

[tool.pyteal.ini]
source-mapper-enabled = true
source-mapper-debug = false

Copy link
Contributor

@ahangsu ahangsu left a 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:

Consider this as an informal approval once we figure out how to resolve the problems above.

pyteal/compiler/sourcemap.py Outdated Show resolved Hide resolved
pyteal/compiler/sourcemap.py Show resolved Hide resolved
pyteal/compiler/sourcemap.py Show resolved Hide resolved
pyteal/compiler/sourcemap.py Outdated Show resolved Hide resolved
@tzaffi
Copy link
Contributor Author

tzaffi commented Mar 1, 2023

I think it is roughly in shape, let me revert back for a few following things:

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.

@ahangsu
Copy link
Contributor

ahangsu commented Mar 7, 2023

Since #684 is merged in, we can resolve the merge conflict at some point, and hopefully we will get this one merged too.

pyteal/compiler/flatten.py Outdated Show resolved Hide resolved
pyteal/compiler/compiler.py Outdated Show resolved Hide resolved
Zeph Grunschlag and others added 3 commits March 7, 2023 14:39
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

pyteal/compiler/compiler.py Outdated Show resolved Hide resolved
@tzaffi tzaffi merged commit d1961a6 into algorand:master Mar 7, 2023
@tzaffi tzaffi mentioned this pull request Mar 17, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Mapper: Poor Mapping of Subroutines
5 participants