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

Make components emissary compatible #6252

Closed
Tracked by #5718 ...
Bobgy opened this issue Aug 6, 2021 · 8 comments · Fixed by #6352
Closed
Tracked by #5718 ...

Make components emissary compatible #6252

Bobgy opened this issue Aug 6, 2021 · 8 comments · Fixed by #6352
Assignees

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Aug 6, 2021

The following 24 components do not have a command in their component yaml:

https://github.com/search?l=YAML&p=3&q=repo%3Akubeflow%2Fpipelines+filename%3Acomponent.yaml+-command&type=Code

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 9, 2021

I think we could easily build a component migration tool which can be called like:

kfp component migrate <path-to-component-yaml>

The CLI does the following:

  1. read the component yaml
  2. verify if command is specified
  3. if not, call docker pull && docker inspect to get the command, modify component yaml to include the command

it might be more helpful migrating a folder of components via:

kfp component migrate --recursive <folder-with-components>

However, there is no good way to identify KFP components, we should probably improve on that.

@chensun
Copy link
Member

chensun commented Aug 10, 2021

Note that some component.yaml from the search query are only for unit testing, and not meant to be runnable, so they may use a dummy image, for example: https://github.com/kubeflow/pipelines/blob/d9c019641ef9ebd78db60cdb78ea29b0d9933008/sdk/python/kfp/v2/compiler_cli_tests/test_data/component_yaml/serving_component.yaml

On the other hand, it's possible that some component yaml aren't showing up if the filename isn't suffixed with "component.yaml" (not sure if all component follows this convention).

I think for the specific actions you described above, maybe a script named something like back_fill_container_command or even a script inlined in a README.md somewhere would be just fine.

The CLI command kfp component migrate sounds too generic, it might give users a wrong idea that it could make any v1 components runnable on v2 (or Vertex Pipeline), which won't be feasible for a number of causes, including: missing type annotation, misuse of placeholders, etc.

@chensun
Copy link
Member

chensun commented Aug 11, 2021

Actually, out of the 24 results, there are 11 false positives:

Unit tests with fake image:

empty file:

graph component:

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 12, 2021

Thanks for figuring them out!
I'll start with a separate CLI tool then, one strong reason I want a tool instead of sth else is that:

docker image inspect -f '{{.Config.Entrypoint}} {{.Config.Cmd}}' hello-world

this depends on pulling the image locally, it will be time consuming to migrate many components. I think migrations should be as easy as possible -- to make users easily stay with the latest versions.

For a CLI tool, we can use docker API to only pull the image digests, that'll be much faster.

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 12, 2021

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 16, 2021

Hmm, getting an image's digest does not seem to include their command and args information.

import docker

client = docker.from_env()
data = client.images.get_registry_data(
    'gcr.io/ml-pipeline/api-server:1.7.0-rc.3'
)
print(data.attrs)

I got:

{'Descriptor': {'mediaType': 'application/vnd.docker.distribution.manifest.v2+json', 'digest': 'sha256:21b2f020425b2a99188c5c80b6eb04722a373b6e2e54e97b723caca1fbd9ddc9', 'size': 2209}, 'Platforms': [{'architecture': 'amd64', 'os': 'linux'}]}

command and args are not there

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 16, 2021

However, according to distribution/distribution#1252 (comment), this should be possible, just not documented.

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 16, 2021

Anyway, I cannot find a container registry generic way to do so. I'll stick with docker pull for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants