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

(Not) Use setup-uv in CI tests workflow #4063

Merged
merged 7 commits into from
Sep 13, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Sep 12, 2024

Summary

Performance benchmark

The following word "native" refers to package installed by native github action setups: setup-Python/micromamba/uv

NOTE: setup-uv installer might provide caching function, so the run time might be slower for the first run

Dependency installation time

Time (in seconds) is reported on the Install pymatgen and dependencies stage for "split 1", excluding conda and optional Ubuntu dependencies

Ubuntu 22.04 Windows 11 MacOS
native mamba + pip 88 235 26
native mamba + uv 85 150 28
native mamba + native uv (first run) 93 254 24
native mamba + native uv (second run) 97 141 23

Conclusion: not much difference between setup-uv and pip install uv, and the M1 MacOS runner is just another level.

Pytest running time

Time (in seconds) is reported for "split 1"

Ubuntu 22.04 Windows 11 MacOS
native mamba + pip 187 262 101
native mamba + uv 181 249 114
native mamba + native uv (first run) 183 258 100
native mamba + native uv (second run) 186 249 100

Conclusion: Actual test running time is largely not influenced (as expected).

Link to above workflow runs

Link
native mamba + pip https://github.com/materialsproject/pymatgen/actions/runs/10824932738
native mamba + uv https://github.com/materialsproject/pymatgen/actions/runs/10824062208
native mamba + native uv (first run) https://github.com/materialsproject/pymatgen/actions/runs/10824811886
native mamba + native uv (second run) https://github.com/materialsproject/pymatgen/actions/runs/10824892797

@DanielYang59 DanielYang59 marked this pull request as ready for review September 12, 2024 06:10
@DanielYang59
Copy link
Contributor Author

@janosh Turns out there isn't much speed advantage for setup-uv over pip install uv, the first time running setup-uv is even slower. Perhaps we just keep things simple (also easy to maintain) and still use pip install uv for now.

@DanielYang59 DanielYang59 changed the title Use setup-uv in CI tests workflow (Not) Use setup-uv in CI tests workflow Sep 12, 2024
@janosh
Copy link
Member

janosh commented Sep 12, 2024

Turns out there isn't much speed advantage for setup-uv over pip install uv, the first time running setup-uv is even slower.

good to know. i wasn't expecting it to be faster on a cold cache though. mainly hoping setup-uv would have good caching and be faster on subsequent runs

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Sep 13, 2024

i wasn't expecting it to be faster on a cold cache though. mainly hoping setup-uv would have good caching and be faster on subsequent runs

Tests on second runs (native mamba + native uv (second run)) didn't show obvious advantage. Probably because our installation stage is not bottlenecked by dependency download, see extracted debug info below:

image

pip install torch (70% of the total time):

2024-09-13 02:33:35.316  torch install starts (`Collecting torch==2.2.1 ...`)
2024-09-13 02:34:05.962  All downloading finished (`Installing collected packages: ...`)
2024-09-13 02:34:42.070 Install finished (`Successfully installed ...`)

uv pip install -e .[extras]:

2024-09-13 02:34:42.691 pymatgen dependency install starts (`DEBUG Found static `pyproject.toml` for: pymatgen`)
2024-09-13 02:34:52.577 DEBUG Building: pymatgen @ file:///home/runner/work/pymatgen/pymatgen
2024-09-13 02:35:10.553 DEBUG Finished building: pymatgen @ file:///home/runner/work/pymatgen/pymatgen

@janosh janosh added performance Some functionality is too slow or regressed github actions Pull requests that update GitHub Actions code ci Continuous integration labels Sep 13, 2024
@janosh
Copy link
Member

janosh commented Sep 13, 2024

thanks so much for your benchmarking! ❤️ that's super useful for the decision at hand and later reference. i agree seems there's not much point in adopting setup-uv.

merging now. thanks for cleaning up the needless build-time deps install

@janosh janosh merged commit 7743ac7 into materialsproject:master Sep 13, 2024
42 of 43 checks passed
@DanielYang59
Copy link
Contributor Author

that's super useful for the decision at hand and later reference

No problem! That's what I was hoping too. Looks like torch is the huge bottleneck, hoping #3826 could have a solution soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration github actions Pull requests that update GitHub Actions code performance Some functionality is too slow or regressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use astral-sh/setup-uv in CI
2 participants