-
Notifications
You must be signed in to change notification settings - Fork 217
Define an object for collecting DSLX interpreter events. #3057
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
base: main
Are you sure you want to change the base?
Conversation
| }; | ||
|
|
||
| // Create a results proto if requested and plumb it through options. | ||
| xls::EvaluatorResultsProto results_proto; |
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.
yay structured logs \o/
cf36eee to
bd7bdf3
Compare
|
Any progress on this? Thanks. |
|
@richmckeever: ping for review ? |
|
@meheff Please rebase this when you can; we can't pull it in for final review until conflicts are resolved. |
bd7bdf3 to
f4868dd
Compare
|
Rebased to head... and changed my notifications for the XLS repo to my personal email account as I kept missing responses on my PRs (my work email might as well be /dev/null). |
ericastor
left a comment
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.
Approving to try to kick off the import.
|
@meheff sorry, I missed this. Can you please add Apache license headers? The migration is failing with: error: Error validating 'verify_match 'Licensed under the Apache Error validating 'verify_match 'Licensed under the Apache License, Version |
f4868dd to
c22d749
Compare
|
Added licenses. |
mikex-oss
left a comment
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.
@meheff I tried patching this internally (most of the comments are just FYI), but I don't actually know why the tests have different start indices. Any ideas?
| ], | ||
| ) | ||
|
|
||
| py_test( |
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.
For future reference, please ensure you load py_test from rules_python, as the builtin is deprecated.
You can also use pytype_strict_contrib_test from https://github.com/google/xls/blob/main/xls/build_rules/py_oss_defs.bzl, which might make sense here, since you added type annotations in the test file.
| from typing import List | ||
|
|
||
| from absl.testing import absltest | ||
| from textwrap import dedent |
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.
FYI, this causes an error in the internal linter. Don't import functions.
| out = _run_dump(path) | ||
| dslx = self._proto_path('prog.x') | ||
| expected = dedent(f""" | ||
| {dslx}:6:0: call_trace_test( |
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.
Hmm this is failing internally.
AssertionError:
- /tmp/tmpu3fpyo24/prog.x:5:0: call_trace_test(
? ^
+ /tmp/tmpu3fpyo24/prog.x:6:0: call_trace_test(
? ^
Haven't looked more closely.
| out = _run_dump(out_path) | ||
| dslx = self._proto_path('prog.x') | ||
| expected = dedent(f""" | ||
| {dslx}:4:0: for_loop_inside_test( |
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.
Similar here:
AssertionError:
- /tmp/tmp1n5rbetg/prog.x:3:0: for_loop_inside_test(
? ^
- /tmp/tmp1n5rbetg/prog.x:4:0: for_loop_inside_test(
? ^
| import os | ||
| import subprocess | ||
| import tempfile | ||
| from typing import List |
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.
For future reference, this is deprecated, since list works for type annotations.
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| from __future__ import annotations |
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.
This appears to be unused.
The newly defined DslxInterpreterEvents gathers assert and trace messages from the DSLX interpreter replacing the previous trace hook mechanism. It stores information in the same proto as the IR interpreter does which enables easier comparison of behavior between the interpreters and the sharing of tools. Also augment the proto with additional information like source information.
Also add a utility dump_call_trace which reads an events proto and prints a structured trace of the calls.