-
Notifications
You must be signed in to change notification settings - Fork 297
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
pod template inplace operations #2899
pod template inplace operations #2899
Conversation
Signed-off-by: Daniel Sola <daniel.sola@union.ai>
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.
Can you add a test to make sure that the pod template does not get mutated?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2899 +/- ##
===========================================
- Coverage 75.65% 40.31% -35.34%
===========================================
Files 199 199
Lines 20765 20708 -57
Branches 2665 2665
===========================================
- Hits 15710 8349 -7361
- Misses 4293 12258 +7965
+ Partials 762 101 -661 ☔ View full report in Codecov by Sentry. |
Should I write the test on |
For simplicity, I would call |
Might need some help from the OSS team on the tests. I'm running into some errors that look like:
|
@dansola let's chat tomorrow. |
Signed-off-by: Daniel Sola <daniel.sola@union.ai>
@thomasjpfan I pushed a test! |
re-ran the failed test. |
Signed-off-by: Daniel Sola <daniel.sola@union.ai>
Signed-off-by: Daniel Sola <daniel.sola@union.ai> Signed-off-by: Katrina Rogan <katroganGH@gmail.com>
Why are the changes needed?
When a constant is used for a pod template, subsequent uses of that pod template will contain information from other tasks that use the same pod template and will overwrite information in the task decorator. Mutation of the pod template object the user passes happens here:
flytekit/flytekit/core/utils.py
Line 195 in 0662bc6
What changes were proposed in this pull request?
In
_serialize_pod_spec
we use a deep copy of the pod template rather than making in place operations on the pod template that the user passes.How was this patch tested?
Right now the above workflow fails because
do_y
usesimage_x
and does not havepandas
.After the deep copy,
do_y
actually usesimage_y
.Check all the applicable boxes
Related PRs
Docs link