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

Add ImageSpec.from_env #2895

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

thomasjpfan
Copy link
Member

Tracking issue

Closes flyteorg/flyte#5889

Why are the changes needed?

Resolve notebook environment mismatch described in flyteorg/flyte#5889

What changes were proposed in this pull request?

This PR adds a ImageSpec.from_env into ImageSpec.

How was this patch tested?

I added unit test to this PR and ran this workflow:

from pathlib import Path

from flytekit import current_context, task, workflow, ImageSpec
from flytekit.types.file import FlyteFile

image_spec = ImageSpec.from_env(
    pinned_packages=["joblib"],
    name="myimage",
    registry="localhost:30000",
    packages=[
        "git+https://github.com/thomasjpfan/flytekit@41a6a74df4bab1b7db4f9ff13eb249e94fe7a2e4"
    ],
    apt_packages=["git"],
)


@task(container_image=image_spec)
def create_file() -> FlyteFile:
    working_dir = Path(current_context().working_directory)

    file = working_dir / "outer_file.txt"
    file.write_text("this is some text")

    return file


@workflow
def main() -> FlyteFile:
    return create_file()

Signed-off-by: Thomas J. Fan <[email protected]>
Comment on lines +351 to +360
if "packages" in kwargs:
packages = kwargs.pop("packages")
else:
packages = []

pinned_packages = pinned_packages or []

for package_to_pin in pinned_packages:
package_version = version(package_to_pin)
packages.append(f"{package_to_pin}=={package_version}")
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be any special handling for when there are overlapping packages in pinned_packages and packages?

e.g. replace the unpinned package with the pinned package and logging via logger.info?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no issue with overlapping unless there is a conflict. For example, this installs flytekit==1.13.8:

uv pip install flytekit flytekit==1.13.8

If there is a conflict, then the builder will error:

uv pip install flytekit==1.13.11 flytekit==1.13.12
  × No solution found when resolving dependencies:
  ╰─▶ Because you require flytekit==1.13.11 and flytekit==1.13.12, we can conclude that your requirements are
      unsatisfiable.

Given that we also have requirements.txt, there can be conflicts there too. I rather have the not do the error checking here.

As an alternative, I considered disallowing packages & requirements here, but it seemed to restrictive.

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 this pull request may close these issues.

[Core feature] Add ImageSpec.from_notebook_env
2 participants