feat!: Support for compilation unit, subprogram and location debug metadata#1554
feat!: Support for compilation unit, subprogram and location debug metadata#1554
Conversation
|
This PR contains breaking changes to the public Python API. Breaking changes summary |
Bencher Report
|
| Branch | ts/debug-info |
| Testbed | Linux |
🚨 2 Alerts
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | hugr_bytes bytes x 1e3 | 📈 plot 🚷 threshold 🚨 alert (🔔) | 154.51 x 1e3(+9.02%)Baseline: 141.73 x 1e3 | 143.15 x 1e3 (107.94%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | hugr_bytes bytes x 1e3 | 📈 plot 🚷 threshold 🚨 alert (🔔) | 18.77 x 1e3(+7.17%)Baseline: 17.52 x 1e3 | 17.69 x 1e3 (106.11%) |
Click to view all benchmark results
| Benchmark | hugr_bytes | Benchmark Result bytes x 1e3 (Result Δ%) | Upper Boundary bytes x 1e3 (Limit %) | hugr_nodes | Benchmark Result nodes (Result Δ%) | Upper Boundary nodes (Limit %) |
|---|---|---|---|---|---|---|
| tests/benchmarks/test_big_array.py::test_big_array_compile | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 154.51 x 1e3(+9.02%)Baseline: 141.73 x 1e3 | 143.15 x 1e3 (107.94%) | 📈 view plot 🚷 view threshold | 6,620.00(0.00%)Baseline: 6,620.00 | 6,686.20 (99.01%) |
| tests/benchmarks/test_ctrl_flow.py::test_many_ctrl_flow_compile | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 18.77 x 1e3(+7.17%)Baseline: 17.52 x 1e3 | 17.69 x 1e3 (106.11%) | 📈 view plot 🚷 view threshold | 581.00(0.00%)Baseline: 581.00 | 586.81 (99.01%) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1554 +/- ##
========================================
Coverage 93.56% 93.56%
========================================
Files 130 133 +3
Lines 12286 12410 +124
========================================
+ Hits 11495 11612 +117
- Misses 791 798 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
|
|
||
| @dataclass | ||
| class FunctionMetadata: |
There was a problem hiding this comment.
This likely isn't the best way for storing this data, but after trying a couple of solutions that were all not ideal, my opinion is it doesn't make sense to overthink the design too much at the moment because this #1393, modifiers, and future debug metadata could all influence the design so for now something that is working is all that's needed
| filename = get_file(node) | ||
| # If we can't fine a file for a node, we default to 0 which corresponds to the | ||
| # entrypoint file. | ||
| file_idx = ctx.metadata_file_table.get_index(filename) if filename else 0 |
There was a problem hiding this comment.
In the case that we are unable to find a file for the node (shouldn't be the case in theory but in practice you could get issues with custom node) I don't think it would be good to error during compilation but rather through whatever consumes the debug data - just setting it to the root file 0 probably isn't the best solution in that case, should it be something like -1 to indicate the issue?
There was a problem hiding this comment.
Yup, it's best to have a specific null value so we can tell the user "unknown filename" rather than give them an incorrect file name. -1 is fine, could also just not include the filename member in the record if it's not found.
There was a problem hiding this comment.
More generally I think this is also the answer to your question about what to do when it's harder to acquire the correct annotation for a node. I would suggest just marking it with some kind of null placeholder, which the backend can report as "debug info missing", and then we can revisit it when the missing information becomes an issue.
There was a problem hiding this comment.
Nice! Thanks @tatiana-s. Just some very broad points.
- I like the proposal to move some of the debug definition into
hugr-py. Not sure if that should includeHugrDebugInfoor just the keys. - Should
turn_on_debug_modejust be an extra argument to.compile(and friends)?
Of course as per your description...the big thing about debug info is remembering to set it everywhere 😒.
-
In time you might make some of your
ast_node: ASTNode | None = Nonenon-optional, or at least remove the default (maybe you could do the latter now). -
I wonder about changing that
builder: DFBase[P]and top-leveldef add_opto a new class wrappingbuilderand exposing its ownadd_op. Then one cannot accidentally call the underlyingbuilder.add_opdirectly (skipping the metadata). -
Finally there is the question (your description hints at this): if a caller to
add_opdoesn't specify anast_node(or passesNone) - we'll end up with no source location on the node at all - is there anything we can do? Can we "inherit" from the nearest enclosing ast-node, say? E.g. if the builder-wrapper from previous, stores the current ast-node as a field and has a context-manager to enter a new ASTNode (overriding the field until you exit the context manager), then if a call toadd_opdoesn't specifyast_nodeexplicitly we could use that from the nearest-enclosing context?
| "test_debug_info.py", | ||
| "metadata_example.py", | ||
| ] | ||
| funcs = hugr.children(hugr.module_root) |
There was a problem hiding this comment.
nit: I prefer patterns where you also check that funcs has the elements you expect. (E.g. if funcs was empty, you'd trivially pass the for loop + match test.)
You could assert names of funcs first, but that duplicates the names between the assert and all the individual cases; better still would be to build a dict from f_name to func, and then pull the functions out one-by-one to make the checks on them. (So, no match.) At the end you can assert there are no functions left in the dict.
| def add_op( | ||
| self, op: ops.DataflowOp, /, *args: Wire, ast_node: AstNode | None = None | ||
| ) -> Node: | ||
| """Adds an op to the builder, with optional debug info.""" |
There was a problem hiding this comment.
is the ast_node the debug info? seems more correct to say "with optional AST node related to the op"
| *args: Wire, | ||
| ast_node: AstNode | None = None, | ||
| ) -> Node: | ||
| """Adds an op to the builder, with optional debug info.""" |
There was a problem hiding this comment.
same comment as above about "debug info"
| hugr_func, *(input_list + bool_wires + param_wires) | ||
| ) | ||
| if debug_mode_enabled(): | ||
| if self.defined_at is not None: |
| return self._node_metadata | ||
|
|
||
| def set_debug_info(self, debug_info: DebugRecord) -> None: | ||
| self._node_metadata[HugrDebugInfo.KEY] = debug_info.to_json() |
There was a problem hiding this comment.
do we need to to_json() at this stage? Why not keep structured and call this at the serialization stage?
|
|
||
| @dataclass | ||
| class DICompileUnit(DebugRecord): | ||
| """Debug information for a compilation unit, corresponds to a module node.""" |
There was a problem hiding this comment.
| """Debug information for a compilation unit, corresponds to a module node.""" | |
| """Debug information for a compilation unit, corresponds to a HUGR module node.""" |
| """Global state for determining whether to attach debug information to Hugr nodes | ||
| during compilation.""" | ||
|
|
||
| DEBUG_MODE_ENABLED = False |
There was a problem hiding this comment.
should this be private?
| DEBUG_MODE_ENABLED = False | |
| _DEBUG_MODE_ENABLED = False |
| # Add debug info about the module to the root node | ||
| if debug_mode_enabled(): | ||
| module_info = DICompileUnit( | ||
| directory=Path.cwd().as_uri(), |
There was a problem hiding this comment.
should this be the cwd or the directory where the file is?
| module_info = DICompileUnit( | ||
| directory=Path.cwd().as_uri(), | ||
| # We know this file is always the first entry in the file table. | ||
| filename=ctx.metadata_file_table.get_index(filename), |
There was a problem hiding this comment.
get appends if not present - do you need to initialise with the file up top?
| meta = hugr.module_root.metadata | ||
| assert HugrDebugInfo in meta | ||
| debug_info = DICompileUnit.from_json(meta[HugrDebugInfo.KEY]) | ||
| assert get_last_uri_part(debug_info.directory) == "guppylang" |
There was a problem hiding this comment.
this wouldn't be true if the tests were run from somewhere else?
| assert lines == [113, 114, 28, 116] | ||
|
|
||
|
|
||
| # TODO: Improve this test. |
There was a problem hiding this comment.
expand on what needs to be improved/raise an issue and link it
6 participants
Closes #1507
Main changes with some points for discussion (+ see comments for further issues in specific files)
Specification for debug records using the Hugr metadata protocol is innow moved to a hugr PR: feat: Add debug info metadata specification inmetadata.debug_info.pyfor now - would it make sense to have this inhugr-pyinstead given the key iscore.debug_info(should at least the key be defined there?)hugr-pyhugr#2971turn_on_debug_mode()/turn_off_debug_mode()to toggle global flag for adding debug metadata during compilation (off by default)add_opmethods defined in the Guppy compiler as opposed to the ones from Hugr directly, as the Guppy ones now include a check for adding debug metadataMakeTupleare getting annotations when used in struct constructors for exampleBREAKING CHANGE: (guppylang-internals) Refactored metadata-related code structure and renamed
GuppyMetadatatoFunctionMetadata