CI: Use uv installer #6363
CI: Use uv installer #6363
Conversation
There was a problem hiding this comment.
Thanks a lot @danielhollas I have been following the uv development closely and it is looking great indeed. Especially since our installation is quite straightforward we shouldn't expect any problems.
So far I've only touched jobs in ci-code.yml. If desired, I can make similar changes to other workflows that would benefit (e.g. pre-commit)
Since the changes pass and there is a significant speed-up, yes please, let's do all other workflows as well 👍
danielhollas
force-pushed
the
uv
branch
2 times, most recently
from
39c98fa to
e895983
Compare
|
Hmm, this check failure is strange and seems unrelated to this PR? https://github.com/aiidateam/aiida-core/actions/runs/8775766327/job/24078477856 (detected asyncio module being loaded in verdi during tab completion? But only for version 3.9, not 3.12?) EDIT: I also don't understand the pre-commit job failure? Maybe uv flake? |
| run: | | ||
| pip install -r requirements/requirements-py-${{ matrix.python-version }}.txt | ||
| pip freeze | ||
| pip install --dry-run --ignore-installed -r requirements/requirements-py-${{ matrix.python-version }}.txt |
There was a problem hiding this comment.
I kept pip here so we have at least some tests that use it.
It seems to be a straigh-up error in Finally, we should also see if we can use a cache for downlaoded packages. This is currently done through the |
This is to speed up installation of dependencies.
danielhollas
force-pushed
the
uv
branch
3 times, most recently
from
879cef5 to
4f1a8e4
Compare
|
Things are going well I see 😂 |
😭 I hate GHA more and more... Anyway, I think this is now ready. 🎉
Turns out this was largely my error, I was trying to install pre-commit extras with uv venv
uv pip install --no-deps -e .[pre-commit]pip in this case simply ignores the extras (which tbh is a fishy behaviour),
Given uv's speed I don't think its worth it, also uv's cache is not really space efficient (uncompressed) so it takes non-trivial amount of time to fetch. See some discussions on this: This comment indicates that having cache might be worthwhile if uv needs to do expensive source dist builds, not sure if that's the case here. astral-sh/uv#1386 (comment) Anyway, I'd leave that for a follow-up PR. 😛 |
| extras: '[pre-commit]' | ||
| from-requirements: 'true' |
There was a problem hiding this comment.
I realize that this was already the case, but why are we installing from requirements and then normally with the extra? I presume because we cannot specify the extra when installing from requirements. But then why not simply get rid of the requirements install. I don't think we need it for the pre-commit step
There was a problem hiding this comment.
I thought so as well, but if I don't install from requirements first the pre-commit will actually fail, I think because it is missing pytest library. I'll open a separate issue for that.
| pip install -r requirements/requirements-py-3.10.txt | ||
| pip install --no-deps -e . |
There was a problem hiding this comment.
Looking at this now, I am not sure why it is first installing from requirements and then doing --no-deps -e .. Do you know why? Does installing with -r somehow not install the package itself? That cannot be, can it? Or is it because that doesn't support the -e flag? But then again, why would we need a linked install for CI?
Just asking to understand if it makes sense that we get rid of this double install approach.
There was a problem hiding this comment.
Does installing with -r somehow not install the package itself? That cannot be, can it?
I just tested that aiida-core indeed does not get installed, which actually makes sense since it is not defined inside the requirements file. So I think the current approach makes sense.
Or is it because that doesn't support the -e flag? But then again, why would we need a linked install for CI?
I also don't think editable install is needed. I am happy to change it, but I think it's better to be done as separate PR, so that here we only focus on uv and don't do any functional changes.
| extras: | ||
| description: aiida-core extras (including brackets) | ||
| default: '' | ||
| required: false | ||
| # NOTE: Hard-learned lesson: we cannot use type=boolean here, apparently :-( | ||
| # https://stackoverflow.com/a/76294014 | ||
| from-requirements: | ||
| description: Install aiida-core dependencies from pre-compiled requirements.txt file | ||
| default: 'true' | ||
| required: false |
There was a problem hiding this comment.
Are these not mutually exclusive? If extras are specified, the install cannot be done from a requirements file? If so, we can just get rid of the from-requirements variable right and just check whether extras is not an empty string.
There was a problem hiding this comment.
Not necessarily, see the case of pre-commit above. But we might want to simplify that in the future.
(Don't tell @mbercx) Co-authored-by: Sebastiaan Huber <[email protected]>
| # If this syntax looks weird to you, dear reader, know that this is | ||
| # GHA's way to do ternary operator. :-/ | ||
| # https://docs.github.com/en/actions/learn-github-actions/expressions#example |
There was a problem hiding this comment.
Just out of curiosity, what part do you find weird? To me the structure (condition) && true || false seems quite logical coming from bash. Then there is the ${{ }} as GHA's outer expression wrapper.
There was a problem hiding this comment.
Thanks a lot @danielhollas Centralizing the install in one action cleans things up nicely as well
uv is the new cool kid in the town of Python packaging, from the creators of ruff. It currently serves as a (much) faster drop-in replacement for pip.
We've been using it for a couple of repos in AiiDAlab and it works great. For aiida-core, it cuts installation time from ~1m30s to ~10s, so it seems worthwhile.
So far I've only touched jobs in
ci-code.yml. If desired, I can make similar changes to other workflows that would benefit (e.g. pre-commit)