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

Use uv-pre-commit to validate lockfile #6699

Merged
merged 7 commits into from
Jan 24, 2025
Merged

Use uv-pre-commit to validate lockfile #6699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ee5c84b
Use uv-pre-commit to validate lockfile
danielhollas Jan 10, 2025
6de1fd0
Bump setup-uv action to 5.2.1
danielhollas Jan 16, 2025
47e5cdd
Fix coverage action warning
danielhollas Jan 17, 2025
ebb2038
Bump uv to 0.5.21
danielhollas Jan 17, 2025
7c2de7a
Bump uv in .readthedocs.yml
danielhollas Jan 18, 2025
290818b
Install latest uv in test-install.yml
danielhollas Jan 22, 2025
daa3fb5
Bump uv to 0.5.22
danielhollas Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/actions/install-aiida-core/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ runs:
python-version: ${{ inputs.python-version }}

- name: Set up uv
uses: astral-sh/[email protected].0
uses: astral-sh/[email protected].1
with:
version: 0.5.x
version: 0.5.22
python-version: ${{ inputs.python-version }}

- name: Install dependencies from uv lock
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/ci-code.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ jobs:
with:
token: ${{ secrets.CODECOV_TOKEN }}
name: aiida-pytests-py3.9
file: ./coverage.xml
files: ./coverage.xml
Copy link
Collaborator Author

@danielhollas danielhollas Jan 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Unrelated change, this was throwing a warning in CI jobs.

fail_ci_if_error: false # don't fail job, if coverage upload fails

tests-presto:
Expand Down
7 changes: 2 additions & 5 deletions .github/workflows/test-install.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,13 @@ jobs:
python-version: '3.11'

- name: Set up uv
uses: astral-sh/[email protected].0
uses: astral-sh/[email protected].1
with:
version: 0.5.x
version: latest
Copy link
Contributor

@agoscinski agoscinski Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

why do we use here latest and not version 0.5.22 as in the install-aiida-core action. Also in that action python-version: ${{ inputs.python-version }} is specified. Should we add here also a python-version: 3.11 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please see: #6699 (comment)

We're only using the uv pip install interface here so we can keep things simple.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also in that action python-version: ${{ inputs.python-version }} is specified. Should we add here also a python-version: 3.11 ?

In the actions case, specifying the python version is needed because it has a side-effect of automatically activating the virtual environment. Again, this is not needed here we can keep things simple and not duplicate the python version information.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see thanks for the clarification


- name: Install utils/ dependencies
run: uv pip install --system -r utils/requirements.txt

- name: Validate uv lockfile
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's better to have this check only in one place.

run: uv lock --check

- name: Validate conda environment file
run: python ./utils/dependency_management.py validate-environment-yml

Expand Down
20 changes: 7 additions & 13 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ ci:
autofix_prs: true
autoupdate_commit_msg: 'Devops: Update pre-commit dependencies'
autoupdate_schedule: quarterly
skip: [mypy, check-uv-lock, generate-conda-environment, validate-conda-environment, verdi-autodocs]
skip: [mypy, generate-conda-environment, validate-conda-environment, verdi-autodocs]

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
Expand Down Expand Up @@ -56,6 +56,12 @@ repos:
environment.yml|
)$

- repo: https://github.com/astral-sh/uv-pre-commit
rev: 0.5.22
hooks:
# Check and update the uv lockfile
- id: uv-lock

- repo: local

hooks:
Expand Down Expand Up @@ -187,18 +193,6 @@ repos:
src/aiida/workflows/arithmetic/multiply_add.py|
)$

- id: check-uv-lock
name: Check uv lockfile up to date
# NOTE: This will not automatically update the lockfile
entry: uv lock --check
language: system
pass_filenames: false
files: >-
(?x)^(
pyproject.toml|
uv.lock|
)$

- id: generate-conda-environment
name: Update conda environment file
entry: python ./utils/dependency_management.py generate-environment-yml
Expand Down
4 changes: 2 additions & 2 deletions .readthedocs.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ build:
# https://docs.readthedocs.io/en/stable/build-customization.html#install-dependencies-with-uv
pre_create_environment:
- asdf plugin add uv
- asdf install uv 0.5.20
- asdf global uv 0.5.20
- asdf install uv 0.5.22
- asdf global uv 0.5.22
create_environment:
- uv venv
install:
Expand Down
6 changes: 5 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,4 +516,8 @@ commands = molecule {posargs:test}
"""

[tool.uv]
required-version = ">=0.5.20"
# NOTE: When you bump the minimum uv version, you also need to change it in:
Copy link
Contributor

@khsrali khsrali Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot @danielhollas

Do we need to update also here? .github/workflows/test-install.yml (where you have astral-sh/[email protected])

Is it possible to simplify this, somehow?
Right now, this has to be updated manually in like 4 places...
Maybe one idea could be to have a hook on pre-commit to read the required version from pyproject.toml and if that doesn't match with other places (action.yml, .readthedocs.yml, etc), would automatically update them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need to update also here? .github/workflows/test-install.yml (where you have astral-sh/[email protected])

Good catch. In this case, I opted to simply always install the latest uv version, since we only use it to uv pip install utils/requirements.txt (i.e. no lockfile operations) so installing latest should be safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it possible to simplify this, somehow?

I've created #6721 as a follow-up, since it is not immediately clear how to improve this, and I don't want to block this PR since it is a clear improvement over status quo. I do agree it's not ideal, but also don't see it as a huge deal. Let's discuss in #6721.

# .pre-commit-config.yaml
# .github/actions/install-aiida-core/action.yml
# .readthedocs.yml
required-version = ">=0.5.21"
Copy link
Contributor

@agoscinski agoscinski Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I did not notice this till now: Why do we accept also version 0.5.21 but bump in the other places the version to 0.5.22? I noticed that the version had be increased for some fixes. Are these fixes already included in 0.5.21?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, all the fixes are in 0.5.21. We don't need to bump the minimum supported version unless really needed.

Loading