Migrate CI dependency installation from pip to uv
#3675
Conversation
janosh
added
ci
|
good news: looks like linux CI dependency install time went from 2.5 to 1 min (2.5x speed up) and windows install time went from 9-14 min to 3-5 min (~3x speed up)! also, pinging a few people in case they want to migrate their repos to
|
|
Amazing! I have a few upstream issues in uv I need fixed before I can switch. Already tried. But I am hopeful it will be soon! |
|
Glad to help you with Despite of the dramatic speed up, I'm quite curious why the original |
|
Well...At lease from this run, |
|
A quick update @janosh , it seems Run uv pip install numpy cython --system
Resolved 2 packages in 98ms
Downloaded 2 packages in 419ms
Installed 2 packages in 17ms
+ cython==3.0.9
+ numpy==1.26.4
Built 1 editable in 28.18s
Resolved 194 packages in 2.55s
error: Failed to download distributions
Caused by: Failed to fetch wheel: torch==2.2.1
Caused by: Failed to extract source distribution
Caused by: Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT (current value: 300s). |
|
good to know, i'll keep an eye on it |
|
|
||
| # TODO remove next line installing ase from main branch until FrechetCellFilter is released | ||
| pip install git+https://gitlab.com/ase/ase | ||
| uv pip install -e '.[dev,optional]' --system |
There was a problem hiding this comment.
@janosh, sorry for digging out this. But why do we need to install "editable" for CI?
There was a problem hiding this comment.
i don't remember exactly. i'm fairly sure i tried non-editable first and it failed for some reason. but that was with an earlier uv and might have changed
There was a problem hiding this comment.
I could confirm install "non-editable" indeed breaks the tests, and install editable works fine. The pip install process seems to run without any issue, but pytest seems unable to access test files. I don't quite understand this behavior though (thought the only difference being able to change source code).
An off-topic question: why do we need:
pymatgen/.github/workflows/test.yml
Lines 18 to 21 in 7064c43
By default workflows would not run on forks until enabled by fork owner. Having this would prevent fork from running workflows, which seems unnecessary (some people might prefer to test on their own fork before pushing to upstream)? Or in my case I have to remove this line to run tests on my own fork.
There was a problem hiding this comment.
editable install basically just symlinks the repo into site-packages, meaning the whole repo contents including test files are symlinked. i think with non-editable install TEST_FILES_DIR points at a non-existent directory.
why do we need:
if: github.repository == 'materialsproject/pymatgen' not sure if this changed but people used to get every CI failure email twice from GitHub, once on their fork and once on open PRs which was very annoying
There was a problem hiding this comment.
editable install basically just symlinks the repo into site-packages, meaning the whole repo contents including test files are symlinked. i think with non-editable install TEST_FILES_DIR points at a non-existent directory.
That's interesting. I might experiment with this later. Because I have a feeling that we don't need install editable when we don't actually edit the source code. And I'm not sure if install editable would have any performance penalty. It's just guessing now but I would test later and let you know :)
not sure if this changed but people used to get every CI failure email twice from GitHub, once on their fork and once on open PRs which was very annoying
I think this is intended (one failure from their own fork, and one from upstream). They should disable workflow on their own fork if they don't want workflow to run on their fork, instead of we disable altogether for them? Otherwise people who want to run workflow on their fork for any reason would need to remove this line?
There was a problem hiding this comment.
i'm guessing if anything, --editable offers a performance gain given how fast it is to symlink
Otherwise people who want to run workflow on their fork for any reason would need to remove this line?
not sure that's an issue? people usually just fork to make PRs in which CI runs on this repo. if they want to maintain a fork long-term, removing this line is not much work?
There was a problem hiding this comment.
i'm guessing if anything,
--editableoffers a performance gain given how fast it is to symlink
Not sure, but I would test later and let you know.
people usually just fork to make PRs in which CI runs on this repo.
Yes. But my point is... By default workflow would NOT run on forks (I assume people would enable workflow on their fork only when they have a good reason to), such that people could easily toggle workflow on and off easily on their fork (and have more control over their fork).
But if we disable workflow for them, they can not easily enable it. (I guess click a button would be easier than changing a line and make a commit? Also if they want to to push their change to upstream, they would have to revert it.)
hoping the migration to
uv, besides being faster thanpip, can also fix the CI import errors formatglandpydanticthat started 2 days ago. not optimistic aboutpydanticwhich shouldn't be imported by pymatgen or any of its deps anyway but maybematgl.related PR: CederGroupHub/chgnet#133