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

Update uv.lock + CI tweaks + fix pymatgen issue #6676

Merged
merged 10 commits into from
Jan 8, 2025
Merged

Update uv.lock + CI tweaks + fix pymatgen issue #6676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b28f5a1
Update uv.lock
danielhollas Dec 28, 2024
475cedb
CI: Update uv
danielhollas Dec 29, 2024
148142a
Run verdi tests from lock
danielhollas Dec 29, 2024
79398fb
Make verdi tests pass
danielhollas Dec 29, 2024
c054b5c
Stupid GHA
danielhollas Dec 29, 2024
33152f9
Update lock
danielhollas Jan 6, 2025
0394eb1
FORCE_COLOR in test-install.yml
danielhollas Jan 6, 2025
a456400
CI: Run validate-dependency-specification with python 3.11
danielhollas Jan 7, 2025
86938af
Unset CI to workaround the pymatgen issue
danielhollas Jan 7, 2025
79c81a7
Fixup VIRTUAL_ENV for python3.9
danielhollas Jan 7, 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
19 changes: 10 additions & 9 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 @@ -7,16 +7,18 @@ inputs:
default: '3.9' # Lowest supported version
required: false
extras:
description: aiida-core extras (including brackets)
default: ''
description: list of optional dependencies
# NOTE: The default 'pre-commit' extra recursively contains
# other extras needed to run the tests.
default: pre-commit
unkcpz marked this conversation as resolved.
Show resolved Hide resolved
required: false
# NOTE: Hard-learned lesson: we cannot use type=boolean here, apparently :-(
# https://stackoverflow.com/a/76294014
# NOTE2: When installing from lockfile, aiida-core and its dependencies
# are installed in a virtual environment located in .venv directory.
# Subsuquent jobs steps must either activate the environment or use `uv run`
from-lock:
description: Install aiida-core dependencies from a uv lock file
description: Install aiida-core dependencies from uv lock file
default: 'true'
required: false

Expand All @@ -29,19 +31,18 @@ runs:
python-version: ${{ inputs.python-version }}

- name: Set up uv
uses: astral-sh/setup-uv@v4
uses: astral-sh/setup-uv@v5
with:
version: 0.5.6
version: 0.5.x
python-version: ${{ inputs.python-version }}

- name: Install dependencies from uv lock
if: ${{ inputs.from-lock == 'true' }}
# NOTE: We're asserting that the lockfile is up to date
# NOTE2: 'pre-commit' extra recursively contains other extras
# needed to run the tests.
run: uv sync --locked --extra pre-commit
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Previously we were not respecting the inputs.extras when using the lockfile. Now we do (at the expense of very ugly Github Actions syntax :-( )

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is inspired from JavaScript so at least not completely random.

Copy link
Member

Choose a reason for hiding this comment

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

in CI will inputs.extras in uv sync --locked ${{ inputs.extras && format('--extra {0}', inputs.extras) || '' }} always be true? Because if it is not pass as input it will set to "pre-commit" as default, no? Then this clause can be simplified.

Never mind, I see extra='' below.

run: uv sync --locked ${{ inputs.extras && format('--extra {0}', inputs.extras) || '' }}
shell: bash

- name: Install aiida-core
if: ${{ inputs.from-lock != 'true' }}
run: uv pip install --system -e .${{ inputs.extras }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since now setup-uv action activates the virtual environment automatically, we no longer need to install into system python.

run: uv pip install -e .${{ inputs.extras }}
shell: bash
19 changes: 14 additions & 5 deletions .github/workflows/ci-code.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,22 @@ jobs:
python-version: ${{ matrix.python-version }}

- name: Setup environment
# Note: The virtual environment in .venv was created by uv in previous step
run: source .venv/bin/activate && .github/workflows/setup.sh
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The updated setup-uv action now activates the virtual environment automatically.

run: .github/workflows/setup.sh

- name: Run test suite
env:
AIIDA_TEST_PROFILE: test_aiida
AIIDA_WARN_v3: 1
# Python 3.12 has a performance regression when running with code coverage
# NOTE1: Python 3.12 has a performance regression when running with code coverage
# so run code coverage only for python 3.9.
run: uv run pytest -n auto --db-backend psql -m 'not nightly' tests/ ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }}
# NOTE2: Unset CI envvar to workaround a pymatgen issue for Python 3.9
# https://github.com/materialsproject/pymatgen/issues/4243
# TODO: Remove a workaround for VIRTUAL_ENV once the setup-uv action is updated
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This issue is described in detail in #6676 (comment).

I'll remove the workaround hopefully soon in a subsequent PR.

# https://github.com/astral-sh/setup-uv/issues/219
run: |
${{ matrix.python-version == '3.9' && 'unset CI' || '' }}
rabbull marked this conversation as resolved.
Show resolved Hide resolved
${{ matrix.python-version == '3.9' && 'VIRTUAL_ENV=$PWD/.venv' || '' }}
pytest -n auto --db-backend psql -m 'not nightly' tests/ ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }}

- name: Upload coverage report
if: matrix.python-version == 3.9 && github.repository == 'aiidateam/aiida-core'
Expand Down Expand Up @@ -122,7 +128,10 @@ jobs:
uses: ./.github/actions/install-aiida-core
with:
python-version: '3.12'
from-lock: 'false'
from-lock: 'true'
# NOTE: The `verdi devel check-undesired-imports` fails if
# the 'tui' extra is installed.
Copy link
Contributor

@agoscinski agoscinski Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This appears now because with the changes in the install-aiida-core we install all extras? Isn't this failure something we should fix later on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So there are two things here:

  1. The extras parameter was added since the default has changed. So that change is necessary.
  2. Now that we respect the extras parameter when using the lock, I think it is better to use the lockfile. You're right that this change here is not necessary, it just makes things more consistent, since now all jobs in ci-code.yml use the lockfile.

Copy link
Contributor

@agoscinski agoscinski Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah sorry for being not clear. I am fine with the changes, just would like to know if we should make an issue and fix it in some later PR

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 this is minor enough that it could go in here, but let me know if you disagree.
I can open an issue about converting more CI jobs to use the lockfile though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that verdi devel check-undesired-imports fails when tui is installed sounds undesirable and sounds like this should be fixed like only check for asyncio if tui is not installed. I am not sure why this happens

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aaah, now I see what you mean. Sorry, I misunderstood your original comment.
Yeah, I'll open an issue for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened #6691 to follow up on this

extras: ''

- name: Run verdi tests
run: |
Expand Down
20 changes: 15 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 @@ -11,6 +11,9 @@ on:
schedule:
- cron: 30 02 * * * # nightly build

env:
FORCE_COLOR: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this has to be added in this PR? I just don't see anything related to it. Is it just to ensure that colors are used for visuals? I am asking because I remember that some tests rely on the color and fail when playing with these options but I don't remember the details anymore.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just an unrelated improvement to add colors to pytest output in GitHub UI.
I am not aware of any tests that would depend on colors, and we're already using this option in many other workflows (e.g. ci-code) so I think this is a safe change.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was something that failed in #6434 when I enforced colors but I cannot check it because the CI result already removed it. But when the test pass it should be okay!


# https://docs.github.com/en/actions/using-jobs/using-concurrency
concurrency:
# only cancel in-progress jobs or runs for the current workflow - matches against branch & tags
Expand All @@ -32,18 +35,18 @@ jobs:
- name: Set up Python 3.11
uses: actions/setup-python@v5
with:
python-version: '3.9'
python-version: '3.11'

- name: Set up uv
uses: astral-sh/setup-uv@v4
uses: astral-sh/setup-uv@v5
with:
version: 0.5.6
version: 0.5.x

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

- name: Validate uv lockfile
run: uv lock --locked
run: uv lock --check
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The --check option is a new alias for --locked which is more intuitive.


- name: Validate conda environment file
run: python ./utils/dependency_management.py validate-environment-yml
Expand Down Expand Up @@ -208,4 +211,11 @@ jobs:
env:
AIIDA_TEST_PROFILE: test_aiida
AIIDA_WARN_v3: 1
run: pytest -n auto --db-backend psql tests -m 'not nightly' tests/
# Unset CI envvar to workaround a pymatgen issue for Python 3.9
# https://github.com/materialsproject/pymatgen/issues/4243
# TODO: Remove a workaround for VIRTUAL_ENV once the setup-uv action is updated
# https://github.com/astral-sh/setup-uv/issues/219
run: |
${{ matrix.python-version == '3.9' && 'unset CI' || '' }}
${{ matrix.python-version == '3.9' && 'VIRTUAL_ENV=$PWD/.venv' || '' }}
pytest -n auto --db-backend psql tests -m 'not nightly' tests/
3 changes: 2 additions & 1 deletion .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,13 @@ repos:
- id: check-uv-lock
name: Check uv lockfile up to date
# NOTE: This will not automatically update the lockfile
entry: uv lock --locked
entry: uv lock --check
language: system
pass_filenames: false
files: >-
(?x)^(
pyproject.toml|
uv.lock|
)$

- id: generate-conda-environment
Expand Down
Loading
Loading