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

[flytekit] Polish - Container ux #6140

Open
2 tasks done
wild-endeavor opened this issue Jan 5, 2025 · 6 comments
Open
2 tasks done

[flytekit] Polish - Container ux #6140

wild-endeavor opened this issue Jan 5, 2025 · 6 comments
Assignees
Labels
backlogged For internal use. Reserved for contributor team workflow.

Comments

@wild-endeavor
Copy link
Contributor

wild-endeavor commented Jan 5, 2025

Container UX

This is a series of tickets to improve the flytekit authoring experience. If any changes are not possible to make in a backwards-compatible way, split it out into a separate ticket.

Argument name

Rename the argument container_image to image in task (and any other relevant decorators, such as dynamic, and eager). container_image is too verbose and the concept of a “container” should be abstracted away from the user.

Image Spec

Rename ImageSpec to just Image. The idea of a Spec should be abstracted away from the users. In their minds, they are just building an image. With union image builder, they don't even need a docker runtime installed.

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@wild-endeavor wild-endeavor added documentation Improvements or additions to documentation untriaged This issues has not yet been looked at by the Maintainers labels Jan 5, 2025
@wild-endeavor wild-endeavor changed the title [Docs] [Draft] [flytekit] Polish - Container ux Jan 5, 2025
@wild-endeavor wild-endeavor added backlogged For internal use. Reserved for contributor team workflow. and removed documentation Improvements or additions to documentation labels Jan 5, 2025
@thomasjpfan
Copy link
Member

I agree with the container_image change.

I'm unsure about renaming ImageSpec. It is not an "Image", but a specification for how to build an image.

@granthamtaylor
Copy link

I'm unsure about renaming ImageSpec. It is not an "Image", but a specification for how to build an image.

I feel like the same could be said of everything else in Flytekit. For example, a @task is not a task, but a specification of a task, no?

From the perspective of a user, an ImageSpec will eventually evaluate to a reference to an Image.

@eapolinario eapolinario removed the untriaged This issues has not yet been looked at by the Maintainers label Jan 16, 2025
@wild-endeavor wild-endeavor changed the title [Draft] [flytekit] Polish - Container ux [flytekit] Polish - Container ux Jan 17, 2025
@JiangJiaWei1103
Copy link
Contributor

#take

@JiangJiaWei1103
Copy link
Contributor

Hi @wild-endeavor,

I agree with these changes. Before proceeding with the renaming, I would like to confirm my understanding:

  1. Rename ImageSpec to Image
  2. Create an alias for ImageSpec as Image to maintain backward compatibility
  3. Update the relevant documentation

Regarding container_image, would it make sense to support both container_image and image (prioritize the new argument for sure) for now, while warning users about the deprecation of container_image in the future?

cc @thomasjpfan @granthamtaylor

Thanks a lot!

@thomasjpfan
Copy link
Member

Yes, we'll need to keep an alias for ImageSpec for Image for backward compatibility.

Also we'll need to support container_image and image with the behavior:

  1. If only container_image is set, show a deprecation warning informing uses of the new parameter.
  2. If both image and container_image is set, raise an error. (They can not both be set)
  3. If only image is set, use that.

@JiangJiaWei1103
Copy link
Contributor

JiangJiaWei1103 commented Jan 21, 2025

Perfect! Thanks for confirming!

After reviewing the codebase, I would like to confirm if the following changes are expected:

  1. Rename ImageSpecBuilder to ImageBuilder and EnvdImageSpecBuilder to EnvdImageBuilder
  2. Rename the folder image_spec/ to image/ and the file image_spec.py to image.py
    • Note: This will affect all import statements
  3. Rename all variables and function names containing the image_spec substring to image, e.g.,
    • image_spec_dict to image_dict
    • update_image_spec_copy_handling to update_image_copy_handling
  4. Ditto, for those containing container_image substring, e.g.,
    • get_registerable_container_image to get_registerable_image

Finally, should we apply all these changes to the tests/ dir as well? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlogged For internal use. Reserved for contributor team workflow.
Projects
Status: Backlog
Development

No branches or pull requests

5 participants