-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Use sentinel to mark dag as removed on reserialization #39825
Use sentinel to mark dag as removed on reserialization #39825
Conversation
@uranusjr here's the task.dag sentinel PR |
27f2623
to
4a1152c
Compare
darn, realized that this needs to be merged first: #39259 (i can't cherry pick the sentinel bit into independent PR just yet) |
4a1152c
to
e3518a4
Compare
67fece5
to
1284974
Compare
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 good for me from general approach but still one Pytest failing.
We don't serialize the dag on the task.dag attr when making RPC calls. By marking it with a sentinel value, we can add understand when we're dealing with a deserialized object, and then re-set the dag attr while skipping some of the extra code applied in the setter.
70a5331
to
4aa024e
Compare
ok pushed some test fixes let's see 🤞 |
green @jscheffl |
For me this looks all reasonable and green. After the term change (I dislike Elided as well :-D) I would rate for LGTM. |
@potiuk let us know if this gets your blessing, when you have a moment. |
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.
Perfect. May it be merged.
We don't serialize the dag on the task.dag attr when making RPC calls. By marking it with a sentinel value, we can add understand when we're dealing with a deserialized object, and then re-set the dag attr while skipping some of the extra code applied in the setter.
When we pass a task over RPC call we can't include the dag as part of the task without running into recursion trouble. So we just "elide" the dag from the task object, require that it be passed separately and then get reattatched to the task after deserialization. We introduce a sentinel so that way we can raise a helpful error if someone tries to access the dag attr after it's been elided. Additionally, the sentinel lets us short circuit some logic in the task.dag setter that is not helpful.
The motivator for this PR is avoiding conditional code like this:
And the idea for this approach came from a discussion with @uranusjr .
The addition of method
get_relevant_upstream_map_indexes
to the pydantic TI model is just a driveby addition and i can split it out if necessary.