feat: implement force push functionality in ImageSpec #2234

jasonlai1218 · 2024-03-03T10:07:00Z

Tracking issue

https://github.com/flyteorg/flyte/issues/

Why are the changes needed?

So that we can easily override an image with the same name.

What changes were proposed in this pull request?

Add a new attribute _is_force_push to the __post_init__
Add a new method force_push to the ImageSpec class
Update the logic in the ImageBuildEngine class to handle _is_force_push attribute in ImageSpec instances

How was this patch tested?

Setup process

py_image_spec = ImageSpec(
    name="test_version_1",
    registry="jasonlai1218",
)
py_image_spec = py_image_spec.force_push()

Screenshots

Check all the applicable boxes

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

Related PRs

Docs link

flyteorg/flyte#5030

…ngine classes - Add a new attribute `is_force_push` to the `ImageSpec` class - Add a new method `force_push` to the `ImageSpec` class - Update the logic in the `ImageBuildEngine` class to handle `is_force_push` attribute in `ImageSpec` instances Signed-off-by: jason.lai <[email protected]>

codecov · 2024-03-03T11:46:59Z

Codecov Report

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

Project coverage is 83.36%. Comparing base (a2888cf) to head (9b6f58f).
Report is 3 commits behind head on master.

Files	Patch %	Lines
flytekit/image_spec/image_spec.py	75.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2234      +/-   ##
==========================================
- Coverage   83.81%   83.36%   -0.45%     
==========================================
  Files         332      309      -23     
  Lines       25093    24021    -1072     
  Branches     3690     3480     -210     
==========================================
- Hits        21032    20026    -1006     
+ Misses       3441     3375      -66     
  Partials      620      620

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

flytekit/image_spec/image_spec.py

- Add a `force_push` method to the `image_spec` class - Assert that `is_force_push` is True in the test function Signed-off-by: jason.lai <[email protected]>

- Add a default value for `is_force_push` in the `ImageSpec` class - Change the message when force pushing an image in the `ImageBuildEngine` class Signed-off-by: jason.lai <[email protected]>

- Update the default image tag in `ic_result_4` variable Signed-off-by: jason.lai <[email protected]>

flytekit/image_spec/image_spec.py

…ildEngine classes - Change `is_force_push` attribute to `_is_force_push` in the `ImageSpec` class - Remove the `is_force_push` parameter from the `force_push` method in the `ImageSpec` class - Update references to `is_force_push` to `_is_force_push` in the `ImageBuildEngine` class Signed-off-by: jason.lai <[email protected]>

- Add a method to enable force push in `ImageSpec` class - Update image tags in test cases in `test_run.py` - Assert that `_is_force_push` is False in `test_image_spec.py` Signed-off-by: jason.lai <[email protected]>

Future-Outlier

@jasonlai1218
Can you help me test the case with force push?
For example

image = ImageSpec(
    name="test-flyte",
    packages=["flytekit"],
    registry="futureoutlier",
)

and add the flag in the ImageSpec

jasonlai1218 · 2024-03-04T14:44:07Z

@Future-Outlier
What does flag in the ImageSpec mean?

jasonlai1218 · 2024-03-04T14:46:38Z

@Future-Outlier like this?

image = ImageSpec(
    name="test-flyte",
    packages=["flytekit"],
    registry="futureoutlier",
    force_push=True,
)

Future-Outlier · 2024-03-04T14:47:34Z

@Future-Outlier like this?

image = ImageSpec(
    name="test-flyte",
    packages=["flytekit"],
    registry="futureoutlier",
    force_push=True,
)

Yes, and please provide screenshot with overwrite message on terminal, thank you

Future-Outlier · 2024-03-04T14:48:01Z

@Future-Outlier What does flag in the ImageSpec mean?

I mean the example of using force_push

Future-Outlier · 2024-03-04T14:49:01Z

I think the interface should be simple, we add 1 more flag in ImageSpec, and it will force push the image to the specified registry.

jasonlai1218 · 2024-03-04T15:03:46Z

@pingsutw Do you have any thoughts on this? Do I need to add flag to ImageSpec interface?

- Change the variable `is_force_push` to `_is_force_push` Signed-off-by: jason.lai <[email protected]>

pingsutw · 2024-03-04T20:06:38Z

we don't need a flag. just use ImageSpec(..).force_push()

flytekit/image_spec/image_spec.py

- Remove the `_is_force_push` attribute from `ImageSpec` class Signed-off-by: jason.lai <[email protected]>

flytekit/image_spec/image_spec.py

- Refactor the `ImageBuildEngine` class to improve readability and maintainability - Update the logic for handling image building and registration - Fix formatting and messaging inconsistencies in the code Signed-off-by: jason.lai <[email protected]>

thomasjpfan

I agree with #2234 (comment) around adding flag to ImageSpec instead of a method. The two use cases I see for force pushing is:

Debugging
User did not pin their dependencies and want to rebuild the image with newer versions.

In both cases, I'll want to enable and then disable force pushing. In either cases, I think a common pattern is to configure it through the environment. Here is what the API looks like for both designs

# Option 1: __init__
spec = ImageSpec(...,
    force_push=os.environ.get("FORCE_PUSH_IMAGE", "1") == "1" 
)

# Option 2: `force_push ` method
spec = ImageSpec(...)

if os.environ.get("FORCE_PUSH_IMAGE", "1") == "1":
    spec = spec.force_push()

I prefer option 1.

thomasjpfan · 2024-03-06T17:27:16Z

flytekit/image_spec/image_spec.py

@@ -67,6 +67,7 @@ class ImageSpec:

    def __post_init__(self):
        self.name = self.name.lower()
+        self._is_force_push = False  # False by default


Do we need to update:

flytekit/flytekit/image_spec/image_spec.py

Line 248 in 632b2c2

def calculate_hash_from_image_spec(image_spec: ImageSpec):

such that the force_push field is not a part of the hash?

- Add a new test for building an existing image with force push Signed-off-by: jason.lai <[email protected]>

Signed-off-by: Kevin Su <[email protected]>

jasonlai1218 · 2024-03-10T07:45:09Z

Thank you very much @thomasjpfan for your advice.
Thanks to @pingsutw for helping with adjustments.
After adjustment, you can use the following two methods to overwrite existing image:

FLYTE_FORCE_PUSH_IMAGE_SPEC=True pyflyte run --remote image_spec.py wf
image = ImageSpec(registry="ghcr.io/flyteorg", packages=["pandas"]).force_push()

cc @Future-Outlier

Future-Outlier · 2024-03-10T08:48:16Z

Thank you very much @thomasjpfan for your advice. Thanks to @pingsutw for helping with adjustments. After adjustment, you can use the following two methods to overwrite existing image:

FLYTE_FORCE_PUSH_IMAGE_SPEC=True pyflyte run --remote image_spec.py wf

image = ImageSpec(registry="ghcr.io/flyteorg", packages=["pandas"]).force_push()

cc @Future-Outlier

LGTM, I'm comfortable with the result.

…spec-force-push

Signed-off-by: jason.lai <[email protected]> Signed-off-by: Kevin Su <[email protected]> Co-authored-by: Kevin Su <[email protected]>

Signed-off-by: jason.lai <[email protected]> Signed-off-by: Kevin Su <[email protected]> Co-authored-by: Kevin Su <[email protected]> Signed-off-by: Jan Fiedler <[email protected]>

jasonlai1218 requested review from wild-endeavor, kumare3, eapolinario, pingsutw and cosmicBboy as code owners March 3, 2024 10:07

dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 3, 2024

pingsutw reviewed Mar 3, 2024

View reviewed changes

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

jasonlai1218 added 3 commits March 3, 2024 20:01

- feat: implement force push functionality in image_spec class

8a0ec90

- Add a `force_push` method to the `image_spec` class - Assert that `is_force_push` is True in the test function Signed-off-by: jason.lai <[email protected]>

refactor: improve handling of force push in image builds

4c2c396

- Add a default value for `is_force_push` in the `ImageSpec` class - Change the message when force pushing an image in the `ImageBuildEngine` class Signed-off-by: jason.lai <[email protected]>

chore: update default image tag variable in ic_result_4

aa364e3

- Update the default image tag in `ic_result_4` variable Signed-off-by: jason.lai <[email protected]>

pingsutw reviewed Mar 4, 2024

View reviewed changes

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

jasonlai1218 added 2 commits March 4, 2024 18:36

feat: enable force push in ImageSpec class and update tests

a688fbe

- Add a method to enable force push in `ImageSpec` class - Update image tags in test cases in `test_run.py` - Assert that `_is_force_push` is False in `test_image_spec.py` Signed-off-by: jason.lai <[email protected]>

Future-Outlier reviewed Mar 4, 2024

View reviewed changes

refactor: refactor variable names across files

c7b4fcb

- Change the variable `is_force_push` to `_is_force_push` Signed-off-by: jason.lai <[email protected]>

Merge branch 'master' into add-image-spec-force-push

5fd9f8f

pingsutw reviewed Mar 5, 2024

View reviewed changes

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

refactor: refactor class attributes in ImageSpec

d3a0fb5

- Remove the `_is_force_push` attribute from `ImageSpec` class Signed-off-by: jason.lai <[email protected]>

Future-Outlier previously approved these changes Mar 6, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by maintainer label Mar 6, 2024

pingsutw previously approved these changes Mar 6, 2024

View reviewed changes

pingsutw reviewed Mar 6, 2024

View reviewed changes

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

flytekit/image_spec/image_spec.py Outdated Show resolved Hide resolved

jasonlai1218 dismissed stale reviews from pingsutw and Future-Outlier via 7471785 March 6, 2024 09:38

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 6, 2024

thomasjpfan reviewed Mar 6, 2024

View reviewed changes

jasonlai1218 and others added 3 commits March 7, 2024 01:43

test: refactor image building and testing process

32d39c7

- Add a new test for building an existing image with force push Signed-off-by: jason.lai <[email protected]>

Merge branch 'master' into add-image-spec-force-push

ab68d7b

Add FLYTE_FORCE_PUSH_IMAGE_SPEC

1cb8860

Signed-off-by: Kevin Su <[email protected]>

pingsutw previously approved these changes Mar 10, 2024

View reviewed changes

Fix tests

7a71b57

Signed-off-by: Kevin Su <[email protected]>

pingsutw dismissed their stale review via 7a71b57 March 10, 2024 05:49

pingsutw approved these changes Mar 10, 2024

View reviewed changes

jasonlai1218 mentioned this pull request Mar 10, 2024

docs: improve documentation for overwriting existing images flyteorg/flyte#5030

Merged

3 tasks

thomasjpfan approved these changes Mar 11, 2024

View reviewed changes

pingsutw approved these changes Mar 11, 2024

View reviewed changes

Merge branch 'master' of github.com:flyteorg/flytekit into add-image-…

9b6f58f

…spec-force-push

pingsutw approved these changes Mar 13, 2024

View reviewed changes

pingsutw merged commit d61e79e into flyteorg:master Mar 13, 2024
45 of 47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement force push functionality in ImageSpec #2234

feat: implement force push functionality in ImageSpec #2234

jasonlai1218 commented Mar 3, 2024 •

edited

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

Future-Outlier left a comment

jasonlai1218 commented Mar 4, 2024

jasonlai1218 commented Mar 4, 2024

Future-Outlier commented Mar 4, 2024

Future-Outlier commented Mar 4, 2024

Future-Outlier commented Mar 4, 2024

jasonlai1218 commented Mar 4, 2024 •

edited

Loading

pingsutw commented Mar 4, 2024

thomasjpfan left a comment

thomasjpfan Mar 6, 2024

jasonlai1218 commented Mar 10, 2024

Future-Outlier commented Mar 10, 2024

feat: implement force push functionality in ImageSpec #2234

feat: implement force push functionality in ImageSpec #2234

Conversation

jasonlai1218 commented Mar 3, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Mar 3, 2024 • edited Loading

Codecov Report

Future-Outlier left a comment

Choose a reason for hiding this comment

jasonlai1218 commented Mar 4, 2024

jasonlai1218 commented Mar 4, 2024

Future-Outlier commented Mar 4, 2024

Future-Outlier commented Mar 4, 2024

Future-Outlier commented Mar 4, 2024

jasonlai1218 commented Mar 4, 2024 • edited Loading

pingsutw commented Mar 4, 2024

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan Mar 6, 2024

Choose a reason for hiding this comment

jasonlai1218 commented Mar 10, 2024

Future-Outlier commented Mar 10, 2024

jasonlai1218 commented Mar 3, 2024 •

edited

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

jasonlai1218 commented Mar 4, 2024 •

edited

Loading