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(sdk/backend): enable parameterization of container images #11404

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HumairAK
Copy link
Collaborator

Resolves #11391

Description of your changes:
This change allows component base images to be parameterized using runtime pipeline parameters. The container images can be specified within an @pipeline decorated function, and takes precedence over the @component(base_image=..) argument.

This change also adds logic to resolve these runtime parameters in the argo driver logic. It also includes resolution steps for resolving the accelerator type which functions the same way but was missing the resolution logic. The resolution logic is a generic workaround solution for any run time pod spec input parameters that cannot be resolved because they cannot be added dynamically in the argo pod spec container template.

Checklist:

@HumairAK HumairAK force-pushed the set_container_img_2 branch 2 times, most recently from 2e8e4fd to ee6677c Compare November 25, 2024 13:55
@github-actions github-actions bot added the ci-passed All CI tests on a pull request have passed label Nov 25, 2024
backend/src/v2/driver/driver.go Outdated Show resolved Hide resolved
backend/src/v2/driver/driver.go Outdated Show resolved Hide resolved
backend/src/v2/driver/util.go Outdated Show resolved Hide resolved
backend/src/v2/driver/util.go Show resolved Hide resolved
sdk/python/kfp/dsl/pipeline_task.py Show resolved Hide resolved
@jgarciao
Copy link

One question: if the user sets the image with set_container_image, is the cache key using that setting?

@HumairAK HumairAK force-pushed the set_container_img_2 branch 2 times, most recently from 252eb8c to 1c2da91 Compare November 25, 2024 17:40
@HumairAK
Copy link
Collaborator Author

One question: if the user sets the image with set_container_image, is the cache key using that setting?

this is a good point, I think we may want to resolve the container image before creating the fingerprint

@github-actions github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 25, 2024
Copy link

@javali-google javali-google left a comment

Choose a reason for hiding this comment

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

Nit: Is the documentation updated as part of different PR?

@HumairAK
Copy link
Collaborator Author

this is a good point, I think we may want to resolve the container image before creating the fingerprint

Actually we don't need to cover this as it's already covered via the inputs field. Since fingerprint is also generated based on input, every image change via runtime parameter input will result in a new cache, which is what we want.

Example:

For this sample pipeline:

@dsl.component(base_image="docker.io/python:3.9.17")
def empty_component(image: str, otherinput: str):
    print("image=" + image + ", otherinput=" + otherinput)

@dsl.pipeline()
def pipeline_accel(img: str, otherinput: str):
    task = empty_component(image=img, otherinput=otherinput)
    task.set_container_image(img)

We have:

image
image

@HumairAK
Copy link
Collaborator Author

HumairAK commented Nov 25, 2024

@javali-google I belive the docs are auto generated and served via readthedocs

@chensun can confirm

my expectation is the docstring here will be auto surfaced like the other task fields here

@HumairAK HumairAK added this to the KFP SDK 2.11 milestone Nov 25, 2024
@jgarciao
Copy link

Actually we don't need to cover this as it's already covered via the inputs field. Since fingerprint is also generated based on input, every image change via runtime parameter input will result in a new cache, which is what we want ...

Thanks for the detailed analysis, @HumairAK !

@jgarciao
Copy link

lgtm

sdk/python/kfp/compiler/compiler_test.py Show resolved Hide resolved
backend/src/v2/driver/driver.go Outdated Show resolved Hide resolved
backend/src/v2/driver/util.go Outdated Show resolved Hide resolved
backend/src/v2/driver/util.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 27, 2024
@HumairAK
Copy link
Collaborator Author

cc @chensun PTAL when you get the chance it would be good to be able to get this in for 2.11

Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jgarciao, leseb
Once this PR has been reviewed and has the lgtm label, please ask for approval from humairak. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions github-actions bot added ci-passed All CI tests on a pull request have passed and removed ci-passed All CI tests on a pull request have passed labels Nov 27, 2024
Copy link
Contributor

@leseb leseb left a comment

Choose a reason for hiding this comment

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

I believe the CHANGELOG needs to be updated too?

This change allows component base images to be parameterized using runtime pipeline parameters. The container images can be specified within an @pipeline decorated function, and takes precedence over the @component(base_image=..) argument.

This change also adds logic to resolve these runtime parameters in the argo driver logic. It also includes resolution steps for resolving the accelerator type which functions the same way but was missing the resolution logic. The resolution logic is a generic workaround solution for any run time pod spec input parameters that cannot be resolved because they cannot be added dynamically in the argo pod spec container template.

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>
@HumairAK
Copy link
Collaborator Author

I believe the CHANGELOG needs to be updated too?

good call, updated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-passed All CI tests on a pull request have passed size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Add ability to parameterize container images
4 participants