-
Notifications
You must be signed in to change notification settings - Fork 181
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
Fix!: include names of decorator argument references when building python env #3687
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
georgesittas
force-pushed
the
jo/extract_decorator_dependencies
branch
from
January 23, 2025 13:38
2b77d3f
to
ff5ad46
Compare
izeigerman
reviewed
Jan 23, 2025
izeigerman
reviewed
Jan 23, 2025
izeigerman
reviewed
Jan 23, 2025
izeigerman
reviewed
Jan 23, 2025
georgesittas
force-pushed
the
jo/extract_decorator_dependencies
branch
2 times, most recently
from
January 27, 2025 14:49
4cedc91
to
48b8b0f
Compare
izeigerman
approved these changes
Jan 27, 2025
georgesittas
force-pushed
the
jo/extract_decorator_dependencies
branch
4 times, most recently
from
January 28, 2025 18:39
906ff2c
to
39d9f31
Compare
tobymao
approved these changes
Jan 29, 2025
georgesittas
force-pushed
the
jo/extract_decorator_dependencies
branch
from
January 29, 2025 18:42
39d9f31
to
4a92b09
Compare
georgesittas
force-pushed
the
jo/extract_decorator_dependencies
branch
from
January 29, 2025 20:16
25d2d6d
to
99a4d4f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #3640
The main difficulty in solving the linked issue was making SQLMesh detect the
stop_after_attempt
reference, in order to extract and inject it into the python environment so it can be (de)serialized:The decorator names themselves were already being picked up in the previous
decorators
helper. In this PR I expand that functionality by also searching for the decorator call arguments' references.I implemented a new visitor class in order to limit the search to only the decorator sub-trees, instead of traversing the whole function tree. Without this, we'd have to do a nested
walk
within the root node traversal loop, leading to unnecessarily revisiting nodes under the decorator sub-trees.Other than that, I also made sure to exclude callable instances of classes, because they can't be serialized. One such example is the
tenacity.Retrying
class (source). Letting these instances into the python environment doesn't "just work" unfortunately, because an error is raised once we reach this section, since there's no__name__
attribute in them.I verified that this fix works for a project with the model of interest, as well as the example project we have under
test_metaprogramming.py
.