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

chore: Install python dependencies with uv in workflows #4086

Merged
merged 9 commits into from
Apr 23, 2024

Conversation

tokoko
Copy link
Collaborator

@tokoko tokoko commented Apr 9, 2024

What this PR does / why we need it:

  • Upgrades setup-python action to latest
  • Disables caching, caches are bound to branches and unit tests run on different PR source branches cannot reuse each other's cache. Only multiple runs on the same PR can.
  • Installs python dependencies with uv

@tokoko tokoko changed the title chore: Speed up unit tests in workflows chore: Install python dependencies with uv in workflows Apr 9, 2024
@tmihalac
Copy link
Contributor

tmihalac commented Apr 10, 2024

  • Disables caching, caches are bound to branches and unit tests run on different PR source branches cannot reuse each other's cache. Only multiple runs on the same PR can.

Maybe this helps somehow with the caches:
Have a look here in the documentation Cache not found for input keys

"The cache action first searches for cache hits for key and the cache version in the branch containing the workflow run. If there is no hit, it searches for restore-keys and the version. If there are still no hits in the current branch, the cache action retries same steps on the default branch"

I think it solves the problem with the caches because in a pull request the merging is usually into master in which the artifacts should already be in, don't you think ?

@tokoko
Copy link
Collaborator Author

tokoko commented Apr 10, 2024

@tmihalac that might be a good idea, unit tests are never run on master, but integration tests are and unit test jobs might be able to reuse caches written by integration tests. Not sure what will happen when two jobs try to update the cache with the same key at the same time though.

@tokoko
Copy link
Collaborator Author

tokoko commented Apr 11, 2024

@tmihalac One concern I still have is that looks like some people's experience with caching and uv on github is that often it might actually slow things down (see here). I have reenabled caches for all workflows except the linter one. We can use that to compare the install runtimes and then make a decision whether we should leave caching in or not... does that sound okay?

@tmihalac
Copy link
Contributor

Sounds good to me.

To me it seems the tests take shorter time, do you think the same ?

@tokoko
Copy link
Collaborator Author

tokoko commented Apr 11, 2024

I'm not too sure actually, just from a sample size of 1, Install Dependencies step seems to take about 30 seconds without a cache hit (https://github.com/feast-dev/feast/actions/runs/8652609326/job/23725916068?pr=4086). When there is a cache hit, the combined runtime of cache retrieval and install for 3.9 is 23 seconds and for 3.10 it is 32 seconds. On top of that there was an initial cache upload step (that's no longer visible now) for the first run which took somewhere around 45 seconds.

P.S. For some reason everything's slower on mac vms 😄

@tokoko
Copy link
Collaborator Author

tokoko commented Apr 17, 2024

@jeremyary I think this is safe to merge

@franciscojavierarceo
Copy link
Member

Seems like a big change... CC @HaoXuAI

@tokoko
Copy link
Collaborator Author

tokoko commented Apr 20, 2024

fwiw, even though uv is brand new, airflow has already switched to using it in their ci, so it's somewhat battle-tested already.

@HaoXuAI
Copy link
Collaborator

HaoXuAI commented Apr 20, 2024

Is the cache totally not making any benefits for build?

@tokoko
Copy link
Collaborator Author

tokoko commented Apr 20, 2024

@HaoXuAI sorry, PR description is misleading now, I have added cache back in to monitor that. tldr version is that, right now it's probably a net loss actually, we are using an older version of cache action, it's slow to upload and sometimes just fails to find the cache even if it exists. With upgraded cache action and pip, it would definitely be of value, no question about it. The question is whether that still is the case with uv instead of pip. uv is much much faster than pip and to me it looks like cache often brings no real benefit, in other words time spent on cache upload/download is roughly the same as a fresh uv install. The current version of this PR disables caching for a linter job only and we can use that to make a final decision later.

@tokoko
Copy link
Collaborator Author

tokoko commented Apr 20, 2024

This run is a good example. cache step seems to work at first, but decides the cache is invalid for some reason. pip install step takes 2.5 minutes without cache and then the job proceeds to waste almost 6 minutes to upload the directory again. For comparison uv install (without cache) is basically always under a minute.

@@ -41,6 +41,11 @@ install-python-ci-dependencies:
pip install --no-deps -e .
python setup.py build_python_protos --inplace

install-python-ci-dependencies-uv:
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm not familiar with uv. but using it to install in the CI, will it have a different result with direct pip install?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For all intents and purposes, no, it's intended to be a drop-in replacement. There's of course some minor differences, pip tends to be more lenient towards packages with invalid metadata for example, version resolution (which we aren't changing in this pr, but I'll do in a follow-up) might also be negligibly different because they are using different resolvers with different heuristics. But at the end of the day, it shouldn't matter for us.

Copy link
Collaborator

@HaoXuAI HaoXuAI left a comment

Choose a reason for hiding this comment

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

LGTM.
like the idea of upgrading the python env tool.

@HaoXuAI HaoXuAI merged commit 6ef7852 into feast-dev:master Apr 23, 2024
14 checks passed
@tokoko tokoko deleted the ci-speedup branch April 25, 2024 09:05
lokeshrangineni pushed a commit to lokeshrangineni/feast that referenced this pull request Apr 25, 2024
* install dependencies in unit-tests with uv

Signed-off-by: tokoko <[email protected]>

* install dependencies in unit-tests with uv

Signed-off-by: tokoko <[email protected]>

* enable caching, change linter job

Signed-off-by: tokoko <[email protected]>

* change local integration tests to uv

Signed-off-by: tokoko <[email protected]>

* change all installs to uv

Signed-off-by: tokoko <[email protected]>

* try adding uv cache

Signed-off-by: tokoko <[email protected]>

* fix lambda cache step name

Signed-off-by: tokoko <[email protected]>

* reenable caches for uv

Signed-off-by: tokoko <[email protected]>

* remove dangling cache step

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>
jeremyary added a commit that referenced this pull request Apr 25, 2024
* chore: Move feast install to docker build in java it tests (#4126)

* chore: Move feast install to docker build in java it tests

Signed-off-by: tokoko <[email protected]>

* remove commented out lines in compose file

Signed-off-by: tokoko <[email protected]>

* make local compose mode default

Signed-off-by: tokoko <[email protected]>

* limit COPY contents

Signed-off-by: tokoko <[email protected]>

* remove requirements.txt from java tests docker image

Signed-off-by: tokoko <[email protected]>

* include pyproject.toml in dockerfile

Signed-off-by: tokoko <[email protected]>

* change links to depends_on

Signed-off-by: tokoko <[email protected]>

* try updating setup-python to v5

Signed-off-by: tokoko <[email protected]>

* pin macos image to macos-12

Signed-off-by: tokoko <[email protected]>

* force rerun

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* chore: Incorporate release 0.37.1 to master (#4127)

* chore(release): release 0.37.1

## [0.37.1](v0.37.0...v0.37.1) (2024-04-17)

### Bug Fixes

* Pgvector patch ([#4108](#4108)) ([1a1f0b1](1a1f0b1))

### Reverts

* Reverts "fix: Using version args to install the correct feast version" ([#4112](#4112)) ([d5ded69](d5ded69)), closes [#3953](#3953)

* chore: Move feast install to docker build in java it tests (#4126)

* chore: Move feast install to docker build in java it tests

Signed-off-by: tokoko <[email protected]>

* remove commented out lines in compose file

Signed-off-by: tokoko <[email protected]>

* make local compose mode default

Signed-off-by: tokoko <[email protected]>

* limit COPY contents

Signed-off-by: tokoko <[email protected]>

* remove requirements.txt from java tests docker image

Signed-off-by: tokoko <[email protected]>

* include pyproject.toml in dockerfile

Signed-off-by: tokoko <[email protected]>

* change links to depends_on

Signed-off-by: tokoko <[email protected]>

* try updating setup-python to v5

Signed-off-by: tokoko <[email protected]>

* pin macos image to macos-12

Signed-off-by: tokoko <[email protected]>

* force rerun

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Co-authored-by: feast-ci-bot <[email protected]>
Co-authored-by: Tornike Gurgenidze <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* chore: Install python dependencies with uv in workflows (#4086)

* install dependencies in unit-tests with uv

Signed-off-by: tokoko <[email protected]>

* install dependencies in unit-tests with uv

Signed-off-by: tokoko <[email protected]>

* enable caching, change linter job

Signed-off-by: tokoko <[email protected]>

* change local integration tests to uv

Signed-off-by: tokoko <[email protected]>

* change all installs to uv

Signed-off-by: tokoko <[email protected]>

* try adding uv cache

Signed-off-by: tokoko <[email protected]>

* fix lambda cache step name

Signed-off-by: tokoko <[email protected]>

* reenable caches for uv

Signed-off-by: tokoko <[email protected]>

* remove dangling cache step

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* feat: Make arrow primary interchange for online ODFV execution (#4143)

* rewrite online flow to use transform_arrow

Signed-off-by: tokoko <[email protected]>

* fix transformation server

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* feat: Feast/IKV online store documentation (#4146)

* feat: Feast/IKV online store documentation

Signed-off-by: Pushkar Gupta <[email protected]>

* functionality matric

Signed-off-by: Pushkar Gupta <[email protected]>

* more changes

Signed-off-by: Pushkar Gupta <[email protected]>

* mount dir

Signed-off-by: Pushkar Gupta <[email protected]>

---------

Signed-off-by: Pushkar Gupta <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* fix: Default value is not set in Redis connection string using environment variable (#4136)

Removed documentation of Redis connection string supporting default values when using environment variables as it isn't supported

Fixes #3669

Signed-off-by: Theodor Mihalache <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* fix: Make sure schema is used when calling `get_table_query_string` method for Snowflake datasource (#4131)

* Fix get_table_query_string method for Snowflake datasource

Signed-off-by: TomSteenbergen <[email protected]>

* Add quotes to table string

Signed-off-by: TomSteenbergen <[email protected]>

---------

Signed-off-by: TomSteenbergen <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* fix: Change checkout action back to v3 from v5 which isn't released yet (#4147)

Signed-off-by: Jeremy Ary <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* changed the code the way mysql container is initialized. Trying to fix the issue - #4128
Also going to check if this change will be resolved in the github actions as well.

Signed-off-by: Lokesh Rangineni <[email protected]>

* reformatted the file to resolve lint error.

Signed-off-by: Lokesh Rangineni <[email protected]>

* reformatted the file to resolve lint error.

Signed-off-by: Lokesh Rangineni <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>
Signed-off-by: Pushkar Gupta <[email protected]>
Signed-off-by: Theodor Mihalache <[email protected]>
Signed-off-by: TomSteenbergen <[email protected]>
Signed-off-by: Jeremy Ary <[email protected]>
Co-authored-by: Tornike Gurgenidze <[email protected]>
Co-authored-by: Jeremy Ary <[email protected]>
Co-authored-by: feast-ci-bot <[email protected]>
Co-authored-by: Pushkar Gupta <[email protected]>
Co-authored-by: Theodor Mihalache <[email protected]>
Co-authored-by: Tom Steenbergen <[email protected]>
lokeshrangineni added a commit to lokeshrangineni/feast that referenced this pull request Apr 25, 2024
…dev#4140)

* chore: Move feast install to docker build in java it tests (feast-dev#4126)

* chore: Move feast install to docker build in java it tests

Signed-off-by: tokoko <[email protected]>

* remove commented out lines in compose file

Signed-off-by: tokoko <[email protected]>

* make local compose mode default

Signed-off-by: tokoko <[email protected]>

* limit COPY contents

Signed-off-by: tokoko <[email protected]>

* remove requirements.txt from java tests docker image

Signed-off-by: tokoko <[email protected]>

* include pyproject.toml in dockerfile

Signed-off-by: tokoko <[email protected]>

* change links to depends_on

Signed-off-by: tokoko <[email protected]>

* try updating setup-python to v5

Signed-off-by: tokoko <[email protected]>

* pin macos image to macos-12

Signed-off-by: tokoko <[email protected]>

* force rerun

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* chore: Incorporate release 0.37.1 to master (feast-dev#4127)

* chore(release): release 0.37.1

## [0.37.1](feast-dev/feast@v0.37.0...v0.37.1) (2024-04-17)

### Bug Fixes

* Pgvector patch ([feast-dev#4108](feast-dev#4108)) ([1a1f0b1](feast-dev@1a1f0b1))

### Reverts

* Reverts "fix: Using version args to install the correct feast version" ([feast-dev#4112](feast-dev#4112)) ([d5ded69](feast-dev@d5ded69)), closes [feast-dev#3953](feast-dev#3953)

* chore: Move feast install to docker build in java it tests (feast-dev#4126)

* chore: Move feast install to docker build in java it tests

Signed-off-by: tokoko <[email protected]>

* remove commented out lines in compose file

Signed-off-by: tokoko <[email protected]>

* make local compose mode default

Signed-off-by: tokoko <[email protected]>

* limit COPY contents

Signed-off-by: tokoko <[email protected]>

* remove requirements.txt from java tests docker image

Signed-off-by: tokoko <[email protected]>

* include pyproject.toml in dockerfile

Signed-off-by: tokoko <[email protected]>

* change links to depends_on

Signed-off-by: tokoko <[email protected]>

* try updating setup-python to v5

Signed-off-by: tokoko <[email protected]>

* pin macos image to macos-12

Signed-off-by: tokoko <[email protected]>

* force rerun

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Co-authored-by: feast-ci-bot <[email protected]>
Co-authored-by: Tornike Gurgenidze <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* chore: Install python dependencies with uv in workflows (feast-dev#4086)

* install dependencies in unit-tests with uv

Signed-off-by: tokoko <[email protected]>

* install dependencies in unit-tests with uv

Signed-off-by: tokoko <[email protected]>

* enable caching, change linter job

Signed-off-by: tokoko <[email protected]>

* change local integration tests to uv

Signed-off-by: tokoko <[email protected]>

* change all installs to uv

Signed-off-by: tokoko <[email protected]>

* try adding uv cache

Signed-off-by: tokoko <[email protected]>

* fix lambda cache step name

Signed-off-by: tokoko <[email protected]>

* reenable caches for uv

Signed-off-by: tokoko <[email protected]>

* remove dangling cache step

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* feat: Make arrow primary interchange for online ODFV execution (feast-dev#4143)

* rewrite online flow to use transform_arrow

Signed-off-by: tokoko <[email protected]>

* fix transformation server

Signed-off-by: tokoko <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* feat: Feast/IKV online store documentation (feast-dev#4146)

* feat: Feast/IKV online store documentation

Signed-off-by: Pushkar Gupta <[email protected]>

* functionality matric

Signed-off-by: Pushkar Gupta <[email protected]>

* more changes

Signed-off-by: Pushkar Gupta <[email protected]>

* mount dir

Signed-off-by: Pushkar Gupta <[email protected]>

---------

Signed-off-by: Pushkar Gupta <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* fix: Default value is not set in Redis connection string using environment variable (feast-dev#4136)

Removed documentation of Redis connection string supporting default values when using environment variables as it isn't supported

Fixes feast-dev#3669

Signed-off-by: Theodor Mihalache <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* fix: Make sure schema is used when calling `get_table_query_string` method for Snowflake datasource (feast-dev#4131)

* Fix get_table_query_string method for Snowflake datasource

Signed-off-by: TomSteenbergen <[email protected]>

* Add quotes to table string

Signed-off-by: TomSteenbergen <[email protected]>

---------

Signed-off-by: TomSteenbergen <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* fix: Change checkout action back to v3 from v5 which isn't released yet (feast-dev#4147)

Signed-off-by: Jeremy Ary <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>

* changed the code the way mysql container is initialized. Trying to fix the issue - feast-dev#4128
Also going to check if this change will be resolved in the github actions as well.

Signed-off-by: Lokesh Rangineni <[email protected]>

* reformatted the file to resolve lint error.

Signed-off-by: Lokesh Rangineni <[email protected]>

* reformatted the file to resolve lint error.

Signed-off-by: Lokesh Rangineni <[email protected]>

---------

Signed-off-by: tokoko <[email protected]>
Signed-off-by: Lokesh Rangineni <[email protected]>
Signed-off-by: Pushkar Gupta <[email protected]>
Signed-off-by: Theodor Mihalache <[email protected]>
Signed-off-by: TomSteenbergen <[email protected]>
Signed-off-by: Jeremy Ary <[email protected]>
Co-authored-by: Tornike Gurgenidze <[email protected]>
Co-authored-by: Jeremy Ary <[email protected]>
Co-authored-by: feast-ci-bot <[email protected]>
Co-authored-by: Pushkar Gupta <[email protected]>
Co-authored-by: Theodor Mihalache <[email protected]>
Co-authored-by: Tom Steenbergen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants