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

Allow for flytekit version to be specified in default image builder #2563

Merged

Conversation

thomasjpfan
Copy link
Member

Why are the changes needed?

With 1.13.0, the default image builder does not allow flytekit to be specified to a specific version. For example, the following will fail:

from flytekit import ImageSpec, task

image1 = ImageSpec(
    packages=["numpy==1.26.4", "flytekit==1.12.3"],
    builder="default",
)


@task(container_image=image1)
def work():
    pass
 pyflyte build build_flyte.py work

with:

0.220   × No solution found when resolving dependencies:
0.221   ╰─▶ Because you require flytekit==1.13.0 and flytekit==1.12.3, we can
0.221       conclude that the requirements are unsatisfiable.

What changes were proposed in this pull request?

With this PR, we check for flytekit first. If it is not detected in requirements, then we inject flytekit into the requirements with the version used during registration.

How was this patch tested?

Unit test was added to this PR.

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
@thomasjpfan thomasjpfan changed the title Thomasjpfan/allow custom flytekit Allow for flytekit version to be specified in default image builder Jul 5, 2024
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Copy link

codecov bot commented Jul 6, 2024

Codecov Report

Attention: Patch coverage is 30.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 50.88%. Comparing base (a728506) to head (5787388).
Report is 3 commits behind head on master.

Files Patch % Lines
flytekit/image_spec/default_builder.py 30.00% 7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (a728506) and HEAD (5787388). Click for more details.

HEAD has 8 uploads less than BASE
Flag BASE (a728506) HEAD (5787388)
9 1
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2563       +/-   ##
===========================================
- Coverage   80.31%   50.88%   -29.44%     
===========================================
  Files         186      183        -3     
  Lines       18880    18736      -144     
  Branches     3915     3697      -218     
===========================================
- Hits        15164     9534     -5630     
- Misses       3059     8710     +5651     
+ Partials      657      492      -165     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomasjpfan thomasjpfan merged commit 1f37fa4 into flyteorg:master Jul 9, 2024
46 of 48 checks passed
fiedlerNr9 pushed a commit that referenced this pull request Jul 25, 2024
…2563)

* Allow for flytekit version to be specified

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

* REV Revert

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

* REV Revert

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

* Adds more comments

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

---------

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Jan Fiedler <jan@union.ai>
mao3267 pushed a commit to mao3267/flytekit that referenced this pull request Jul 29, 2024
…lyteorg#2563)

* Allow for flytekit version to be specified

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

* REV Revert

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

* REV Revert

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

* Adds more comments

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

---------

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: mao3267 <chenvincent610@gmail.com>
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.

2 participants