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

feat: Support ImageSpec as base image #2277

Merged
merged 2 commits into from
Mar 26, 2024
Merged

Conversation

pingsutw
Copy link
Member

Tracking issue

NA

Why are the changes needed?

It enables users to easily reuse the ImageSpec and merge different ImageSpecs together.

What changes were proposed in this pull request?

ImageBuildEngine will build the base image if the base image is an ImageSpec.

How was this patch tested?

pyflyte run --remote ..

Setup process

from flytekit import task, workflow, ImageSpec, dynamic

new_flytekit = "git+https://github.com/flyteorg/flytekit@64730c8ffb4e76cb268817e30d31c6d209452e75"
image_sklearn = ImageSpec(packages=["scikit-learn", new_flytekit], apt_packages=["git"], registry="pingsutw")
image_tensorflow = ImageSpec(base_image=image_sklearn, packages=["tensorflow"], registry="pingsutw")


@task(container_image=image_sklearn)
def t1(a: int) -> int:
    return a + 2


@workflow
def wf(a: int = 3):
    t1(a=a)


if __name__ == "__main__":
    print(f"Running my_wf: {wf(a=50)}")

Screenshots

Check all the applicable boxes

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

Related PRs

NA

Docs link

NA

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 18, 2024
Signed-off-by: Kevin Su <[email protected]>
@pingsutw pingsutw changed the title feat: Support ImageSpec as base_image feat: Support ImageSpec as base image Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

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

Project coverage is 83.31%. Comparing base (3f45131) to head (edbaf85).
Report is 4 commits behind head on master.

Files Patch % Lines
flytekit/image_spec/image_spec.py 16.66% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2277      +/-   ##
==========================================
- Coverage   83.79%   83.31%   -0.49%     
==========================================
  Files         332      309      -23     
  Lines       25147    24057    -1090     
  Branches     3703     3493     -210     
==========================================
- Hits        21071    20042    -1029     
+ Misses       3450     3387      -63     
- Partials      626      628       +2     

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

@pingsutw
Copy link
Member Author

cc @cjidboon94 This PR can address your issue as well. you can have an ImageSpec like

image1 = ImageSpec(packages=["scikit-learn"], pip_index=<index1>)
image2 = ImageSpec(base_image=image1, packages=["tensorflow"], pip_index=<index2>)

@cjidboon94
Copy link
Contributor

cc @cjidboon94 This PR can address your issue as well. you can have an ImageSpec like

image1 = ImageSpec(packages=["scikit-learn"], pip_index=<index1>)
image2 = ImageSpec(base_image=image1, packages=["tensorflow"], pip_index=<index2>)

While it would solve it, it feels wasteful if image1 is not going to be reused by other tasks/imagespecs definitions (this would be the case for me).

Copy link
Member

@Future-Outlier Future-Outlier left a comment

Choose a reason for hiding this comment

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

TEST

@dosubot dosubot bot added the lgtm This PR has been approved by maintainer label Mar 21, 2024
@pingsutw pingsutw merged commit 71df7d6 into master Mar 26, 2024
45 of 47 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
Signed-off-by: Kevin Su <[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
lgtm This PR has been approved by maintainer size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants