Skip to content
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: Handle overriding of container image in backend #2176

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

fg91
Copy link
Member

@fg91 fg91 commented Feb 7, 2024

Tracking issue

flyteorg/flyte#4543

Why are the changes needed?

The container_image task node override, among others, is currently broken (doesn't work with pyflyte register and only once with pyflyte run, see tracking issue.

What changes were proposed in this pull request?

Don't override container image of run_entity in .with_overrides but actually in flyteidl's TaskNodeOverride so that the container image is overridden in the backend.

How was this patch tested?

In combination with flyteorg/flyte#4858 I ensured that the resulting pod in the cluster has the correct image. I also adapted the unit tests.

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

flyteorg/flyte#4858

Docs link

@fg91
Copy link
Member Author

fg91 commented Feb 9, 2024

Tests are failing because an idl upgrade is needed once flyteorg/flyte#4858 is merged.

@fg91 fg91 changed the title [WIP] Fix: Handle overriding of container image in backend Fix: Handle overriding of container image in backend Feb 9, 2024
@fg91 fg91 requested a review from pingsutw February 9, 2024 20:44
@fg91 fg91 added the bug Something isn't working label Feb 9, 2024
@fg91 fg91 marked this pull request as ready for review February 9, 2024 21:58
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 9, 2024
Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's merge flyteidl first.

@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Feb 14, 2024
Copy link

codecov bot commented Feb 29, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.65%. Comparing base (90324e0) to head (e68a4e8).
Report is 1 commits behind head on master.

Files Patch % Lines
flytekit/models/core/workflow.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2176      +/-   ##
==========================================
- Coverage   85.86%   84.65%   -1.21%     
==========================================
  Files         314      314              
  Lines       24042    24048       +6     
  Branches     3643     3644       +1     
==========================================
- Hits        20643    20359     -284     
- Misses       2759     3037     +278     
- Partials      640      652      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Fabio Grätz added 2 commits February 29, 2024 22:24
Signed-off-by: Fabio Grätz <[email protected]>
@fg91 fg91 force-pushed the fg91/fix/handle-container-image-override-in-backend branch from 26be842 to e68a4e8 Compare February 29, 2024 21:25
@fg91 fg91 merged commit eb20459 into master Mar 1, 2024
87 of 92 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
* Handle overriding of container image in backend

Signed-off-by: Fabio Grätz <[email protected]>

* Adapt tests

Signed-off-by: Fabio Grätz <[email protected]>

---------

Signed-off-by: Fabio Grätz <[email protected]>
Co-authored-by: Fabio Grätz <[email protected]>
Signed-off-by: Jan Fiedler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lgtm This PR has been approved by maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants