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.

Sign up for GitHub

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

Jump to bottom

Store protos in local cache #3022

Merged
merged 2 commits into from
Dec 26, 2024
Merged

Store protos in local cache #3022

merged 2 commits into from
Dec 26, 2024

Conversation

eapolinario
Copy link
Collaborator

Tracking issue

Closes flyteorg/flyte#6125

Why are the changes needed?

Values stored in the local cache in flytekit<1.13.6 cannot be used in flytekit>=1.13.6.

What changes were proposed in this pull request?

This PR changes the values stored in the local cache to be the serialized bytes of the Literal proto message instead of the model file Literal object. This way if we end up adding new fields to the Literal proto message we are protected from the error described in the github issue being fixed by this PR.

Notice that this change makes values produced by the new versions of flytekit to be backwards incompatible (i.e. values written to the local cache will not be read by older versions of flytekit).

How was this patch tested?

Unit tests and local runs across a diverse set of versions.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@eapolinario
Store proto obj instead of model Literal in local cache
c01c460 
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
@eapolinario
Remove unused file
cc9f961 
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
@codecov Codecov
Copy link

codecov bot commented Dec 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.45%. Comparing base (e9a7da1) to head (cc9f961).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
flytekit/core/local_cache.py 86.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3022      +/-   ##
==========================================
- Coverage   79.52%   75.45%   -4.08%     
==========================================
  Files         201      201              
  Lines       21250    21203      -47     
  Branches     2729     2732       +3     
==========================================
- Hits        16900    15999     -901     
- Misses       3574     4391     +817     
- Partials      776      813      +37

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

pingsutw
pingsutw approved these changes Dec 26, 2024
View reviewed changes
flytekit/models/literals.py Show resolved Hide resolved
@eapolinario eapolinario merged commit 9b94910 into master Dec 26, 2024
102 of 104 checks passed
eapolinario added a commit that referenced this pull request Dec 26, 2024
@eapolinario
Store protos in local cache (#3022)
0ad8c4a 
* Store proto obj instead of model Literal in local cache

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Remove unused file

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
@eapolinario eapolinario mentioned this pull request Dec 26, 2024
Merged
eapolinario added a commit that referenced this pull request Dec 26, 2024
@eapolinario @pingsutw
@thomasjpfan @pvditt
Backport changes to 1.14.3 (#3019)
b2a1b2a 
* vscode decorator for the dynamic task (#2994)

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Use correct name in flyteagent test (#3011)

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* fix: Improve error details and logging config handling (#3012)

Signed-off-by: Kevin Su <pingsutw@apache.org>

* Add python interpreter into vscode settings (#3014)

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

* add support for toggling data mode for array node (#2940)

* add support for toggling data mode for array node

Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>

* clean up

Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>

* clean up

Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>

* cleanup

Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>

* Bump flyteidl lower-bound to 1.14.1

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Add import of FlyteLaunchPlan back

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Store protos in local cache (#3022)

* Store proto obj instead of model Literal in local cache

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Remove unused file

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Signed-off-by: Paul Dittamo <pvdittamo@gmail.com>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Paul Dittamo <37558497+pvditt@users.noreply.github.com>
granthamtaylor added a commit that referenced this pull request Jan 6, 2025
@granthamtaylor @eapolinario
@dependabot @pimdh @pingsutw @JiangJiaWei1103 @vincent0426 @Future-Outlier @thomasjpfan @dansola
Rebase (#3038)
8d5bd85 
* Store protos in local cache (#3022)

* Store proto obj instead of model Literal in local cache

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Remove unused file

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Bump aiohttp from 3.9.5 to 3.10.11 (#3018)

Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.9.5 to 3.10.11.
- [Release notes](https://github.com/aio-libs/aiohttp/releases)
- [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst)
- [Commits](aio-libs/aiohttp@v3.9.5...v3.10.11)

---
updated-dependencies:
- dependency-name: aiohttp
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix bug in FlyteDirectory.listdir on local files (#2926)

* Fix issue in FlyteDirectory.listdir

Fixes flyteorg/flyte#6005

Signed-off-by: Pim de Haan <pim@cusp.ai>

* Added test

Signed-off-by: Pim de Haan <pim@cusp.ai>

* Run make lint

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Pim de Haan <pim@cusp.ai>
Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Fix unit tests in airflow plugin (#3024)

Signed-off-by: Kevin Su <pingsutw@apache.org>

* fix: Fix resource meta typos for async agent (#3023)

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* fix: format commands output (#3026)

* Fix pydantic basemodel default input (#3013)

* Fix pydantic default input

Signed-off-by: Future-Outlier <eric901201@gmail.com>

* add pydantic integration test

Signed-off-by: Future-Outlier <eric901201@gmail.com>

* Use duck typing by Thomas's advice

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

* lint

Signed-off-by: Future-Outlier <eric901201@gmail.com>

---------

Signed-off-by: Future-Outlier <eric901201@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

* [BUG] Open FlyteFile from remote path (#2991)

* fix: Open FlyteFile from remote path

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Add integration test

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* refactor: Use ctx as param instead of recreation

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* refactor: Clean test logic

1. Remove redundant prints
2. Use `mock.patch.dict` to setup `os.environ` for the current test fn
    * Avoid contaminating other tests running in the same process

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* refactor: Setup local path and downloader in constructor

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* refactor: Move SimpleFileTransfer to an utility file

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Remove redundant env var setup

Please refer to #3001

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* test: Add another ff use case

Create ff in one task pod and read it in another task pod.

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

---------

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* vllm inference plugin (#2967)

* vllm inference plugin

Signed-off-by: Daniel Sola <daniel.sola@union.ai>

* fixed default value

Signed-off-by: Daniel Sola <daniel.sola@union.ai>

---------

Signed-off-by: Daniel Sola <daniel.sola@union.ai>

* Add poetry to image spec (#3025)

* Add poetry to image spec

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

* Add stricter check

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

---------

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

* [test] Add integration test for accessing sd sttr in dc (#2969)

* test: Add integration test for attr access of sd

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Correct file path

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* test: Support interaction with minio s3 bucket

1. Upload a local parquet file to minio s3 bucket
2. Access StructuredDataset attr from a dataclass
3. Open StructuredDataset from a remote path

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Delete an unmerged integration test

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Try imagespec with commit sha of corresponding fix

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Remove redundant test

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Remove default_factory and create sd dc from input uri

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* refactor: Clean test logic

1. Remove redundant prints
2. Use `mock.patch.dict` to setup `os.environ` for the current test fn
    * Avoid contaminating other tests running in the same process

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Remove redundant minio env var setup and add test comments

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Support uploading tmp pqt file

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Udpate deprecated module

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

* Remove redundant and unused imports

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

---------

Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Pim de Haan <pim@cusp.ai>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com>
Signed-off-by: Future-Outlier <eric901201@gmail.com>
Signed-off-by: Daniel Sola <daniel.sola@union.ai>
Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Eduardo Apolinario <653394+eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pim de Haan <pimdehaan@gmail.com>
Co-authored-by: Kevin Su <pingsutw@apache.org>
Co-authored-by: 江家瑋 <36886416+JiangJiaWei1103@users.noreply.github.com>
Co-authored-by: V <0426vincent@gmail.com>
Co-authored-by: Han-Ru Chen (Future-Outlier) <eric901201@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Daniel Sola <40698988+dansola@users.noreply.github.com>
shuyingliang pushed a commit to shuyingliang/flytekit that referenced this pull request Jan 11, 2025
@eapolinario @shuyingliang
Store protos in local cache (flyteorg#3022)
f22b214 
* Store proto obj instead of model Literal in local cache

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

* Remove unused file

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>

---------

Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com>
Signed-off-by: Shuying Liang <shuying.liang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pingsutw pingsutw pingsutw approved these changes

@wild-endeavor wild-endeavor Awaiting requested review from wild-endeavor wild-endeavor is a code owner

@kumare3 kumare3 Awaiting requested review from kumare3 kumare3 is a code owner

@cosmicBboy cosmicBboy Awaiting requested review from cosmicBboy cosmicBboy is a code owner

@samhita-alla samhita-alla Awaiting requested review from samhita-alla samhita-alla is a code owner

@thomasjpfan thomasjpfan Awaiting requested review from thomasjpfan thomasjpfan is a code owner

@Future-Outlier Future-Outlier Awaiting requested review from Future-Outlier Future-Outlier is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Cached values produced by flytekit<1.13.6 cannot be used in flytekit>=1.13.6
2 participants
@eapolinario @pingsutw