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

[BUG] Register workflow by image sha256 #5132

Closed
2 tasks done
gilbert-stripe opened this issue Mar 28, 2024 · 8 comments · Fixed by flyteorg/flytekit#2335
Closed
2 tasks done

[BUG] Register workflow by image sha256 #5132

gilbert-stripe opened this issue Mar 28, 2024 · 8 comments · Fixed by flyteorg/flytekit#2335
Assignees
Labels
bug Something isn't working flytekit FlyteKit Python related issue

Comments

@gilbert-stripe
Copy link

Describe the bug

Flyte supports passing image while registering workflow by pyflyte. However, it requires passing image tag and hasn't supported image sha256.

Example:
cmd: PYTHONPATH=/pay/src/zoolander pyflyte register src/python/flyte/ml_exploration/dynamic_resource_allocation_example -p ml-exploration -d adhoc -i "containers.global.qa.stripe.io/stripe-qa/flyte/flyte-workflow-minimal-images-2@sha256:1f5b27f07f2ee6c68cb480562dad950184c9bcac590c4f6b2fa220c9173e4f3b" -v gilbert3-123456 --deref-symlinks

Error:
AssertionError: Incorrectly formatted image containers.global.qa.stripe.io/stripe-qa/flyte/flyte-workflow-minimal-images-2@sha256:1f5b27f07f2ee6c68cb480562dad950184c9bcac590c4f6b2fa220c9173e4f3b, missing tag value

Is it possible to support image sha256 in pyflyte?

Expected behavior

Support image sha 256 when registering workflow by pyflyte.

Additional context to reproduce

No response

Screenshots

No response

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

  • Yes

Have you read the Code of Conduct?

  • Yes
@gilbert-stripe gilbert-stripe added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels Mar 28, 2024
Copy link

welcome bot commented Mar 28, 2024

Thank you for opening your first issue here! 🛠

Copy link

dosubot bot commented Mar 28, 2024

Hey @gilbert-stripe, great to see you back! Hope you're doing well.👋

Thank you for bringing this to our attention. Currently, Flyte's pyflyte register command does not support specifying images by their SHA256 digest. The command expects an image tag to be provided with the -i or --image option, as you've noticed.

The error message you're seeing is due to an assertion in the code that checks for the presence of a tag in the image string. When you provide an image digest instead of a tag, this assertion fails, resulting in the error.

To support image digests, changes would need to be made to the ImageSpec class and the pyflyte register command to accept and handle image digests. This would involve modifying the image building logic to use the digest to reference the image instead of using tags, and ensuring that the digest is used in place of a tag when pulling or checking for the existence of the image.

Here's an illustrative example of how the ImageSpec class definition might be modified to include an image_digest parameter:

class ImageSpec:
    def __init__(self, base_image=None, packages=None, python_version=None, apt_packages=None, env=None, registry=None, image_digest=None):
        self.base_image = base_image
        self.packages = packages or []
        self.python_version = python_version
        self.apt_packages = apt_packages or []
        self.env = env or {}
        self.registry = registry
        self.image_digest = image_digest  # New parameter for image digest

    # Existing methods and new methods to handle image specification by digest...

In this example, the image_digest parameter is added to the ImageSpec class constructor, allowing users to specify an image by its digest. The rest of the class would need to be adjusted accordingly to use this image_digest when building, fetching, or checking the image, ensuring that the digest is appropriately utilized in operations involving the container registry.

However, please note that this is a significant change that would need to be thoroughly tested to ensure it doesn't introduce any regressions or break existing functionality. It would also need to be carefully designed to maintain backwards compatibility with existing workflows that specify images by tag.

I hope this provides some clarity on the issue. If you have any further questions, please don't hesitate to ask.

Sources

About Dosu

This response is meant to be useful and save you time. It is not meant to be a precise solution, but rather a starting point for your own research.

Help me be more useful! Please leave a 👍 if this is helpful and 👎 if it is irrelevant.

If you want to continue the conversation, start your reply with @dosu-bot.

@wild-endeavor wild-endeavor self-assigned this Mar 28, 2024
@wild-endeavor wild-endeavor removed the untriaged This issues has not yet been looked at by the Maintainers label Mar 28, 2024
@eapolinario eapolinario self-assigned this Apr 4, 2024
@Tom-Newton
Copy link
Contributor

I'm also very interested in supporting this, though I think my error is actually a bit different.

AssertionError: Incorrectly formatted image containers.global.qa.stripe.io/stripe-qa/flyte/flyte-workflow-minimal-images-2@sha256:1f5b27f07f2ee6c68cb480562dad950184c9bcac590c4f6b2fa220c9173e4f3b, missing tag value 

Comes from https://github.com/flyteorg/flytekit/blob/4c6e704eb600b4fed38434bfe1e68b55ad7fac19/flytekit/configuration/__init__.py#L221 but this actually seems to work for me. Looking at the implementation of parse_repository_tag it looks to me like this part should work.

The problem I have is that the : gets replaced with an @ in Image.full and get_registerable_container_image. I created a draft PR that adds sha256 test cases in a bunch of relevant tests flyteorg/flytekit#2335.

@Tom-Newton
Copy link
Contributor

@eapolinario have you started working this? I am very keen to get it fixed fast so if you are not working on it I can probably do it.

@eapolinario
Copy link
Contributor

@eapolinario have you started working this? I am very keen to get it fixed fast so if you are not working on it I can probably do it.

Go for it. You can tag me on the PR. 😄

@Tom-Newton
Copy link
Contributor

Tom-Newton commented Apr 8, 2024

@gilbert-stripe which version of flytekit are you using? I can't reproduce your error on the latest version. I suspect its because flyteorg/flytekit#1978 recently change the library used for parsing the image identifier string.

@gilbert-stripe
Copy link
Author

@Tom-Newton Hi!
I believe the version is 1.3.2. We got that upgrade to 1.9.1.
Will that resolve the issue?

@Tom-Newton
Copy link
Contributor

Tom-Newton commented Apr 8, 2024

flyteorg/flytekit#1978 only only solves a tiny bit of the issue. I have made a PR that should fix the rest flyteorg/flytekit#2335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working flytekit FlyteKit Python related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants