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

danielhollas · 2024-12-29T19:00:02Z

I've recreated the uv lockfile (uv.lock) with a newer uv version that uses an updated resolution strategy, which in particular solves an issue reported by @agoscinski in #6640 (comment)

I had an issue with scipy, needed to be 1.14.1 for the python 3.13 but uv lock and uv lock --upgrade did not upgrade even though my local version was 1.14.1, then with uv lock --upgrade-package scipy==1.14.1 it finally told me that it does not upgrade because scipy==1.14.1 does not support python 3.9. So I needed to specify the scipy version for the python 3.13 separately.

uv produces a "universal" lockfile for all python versions that we support, i.e >=3.9.
Previously, it tried to find a version of the package that would be compatible with all supported Python versions, which in turn meant that if a package dropped support for 3.9 for example, we'd be stuck on an old version.
In the new uv version, this is not the case, and uv will try to find the newest version of a package for a given python version.

Incidentally, this also help partly resolve the pymatgen issue, see #6680. The issue has been fixed in a new version of monty package, which however does not support Python 3.9. So in the updated lockfile, the issue is fixed for Python >=3.10.

For Python 3.9, I added a workaround suggested in materialsproject/pymatgen#4243 (comment), and unset the CI environmental variable.
Since this woraround is quite targeted (only in two specific job steps, and only for Python 3.9), I believe this is acceptable, and we will be dropping Python 3.9 support sometimes this year anyway.

codecov · 2024-12-29T20:21:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.94%. Comparing base (02cbe0c) to head (79c81a7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6676   +/-   ##
=======================================
  Coverage   77.94%   77.94%           
=======================================
  Files         563      563           
  Lines       41761    41761           
=======================================
  Hits        32545    32545           
  Misses       9216     9216

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

danielhollas · 2024-12-30T22:27:53Z

There's one failing test in test-install with Python 3.9, due to astral-sh/setup-uv#219

        # In Cygwin paths like "c:\..." and '\cygdrive\c\...' are possible
>       if p_venv.parts[1] == "cygdrive":
E       IndexError: tuple index out of range

.venv/lib/python3.9/site-packages/IPython/core/interactiveshell.py:890: IndexError
========================================================== slowest 50 durations ===========================================================

(3 durations < 1s hidden.  Use -vv to show these durations.)
========================================================= short test summary info =========================================================
FAILED tests/tools/ipython/test_ipython_magics.py::test_ipython_magics - IndexError: tuple index out of range

EDIT: This was now resolved upstream, but was not release yet. I've added a workaround in the meantime so we can move in with this PR.

danielhollas · 2024-12-30T23:08:41Z

.github/workflows/ci-code.yml

@@ -64,8 +64,7 @@ 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


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

danielhollas · 2025-01-07T13:42:23Z

@unkcpz @agoscinski this is now ready for review, please take a look, I've written an extended PR description, let me know if anything is not clear. Note in particular:

For Python 3.9, I added a workaround suggested in materialsproject/pymatgen#4243 (comment), and unset the CI environmental variable.
Since this woraround is quite targeted (only in two specific job steps, and only for Python 3.9), I believe this is acceptable, and we will be dropping Python 3.9 support sometimes this year anyway.

Regardless if we decide if this is a good solution or not, I think this PR should go in first before #6680 since it fixes the pymatgen issue for everything except Python 3.9.

danielhollas · 2025-01-07T13:54:44Z

.github/actions/install-aiida-core/action.yml


  - 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


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

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

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.

danielhollas · 2025-01-07T13:55:19Z

.github/actions/install-aiida-core/action.yml

    shell: bash

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


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

danielhollas · 2025-01-07T13:56:28Z

.github/workflows/ci-code.yml

-      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


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

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

danielhollas · 2025-01-07T13:57:08Z

.github/workflows/test-install.yml


    - 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


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

.github/workflows/ci-code.yml

agoscinski

Looks good to me, just need some clarifications with some changes

agoscinski · 2025-01-08T08:02:05Z

.github/actions/install-aiida-core/action.yml


  - 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


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

agoscinski · 2025-01-08T08:16:11Z

.github/workflows/test-install.yml

@@ -11,6 +11,9 @@ on:
  schedule:
  - cron: 30 02 * * *      # nightly build

+env:
+  FORCE_COLOR: 1


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.

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.

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!

agoscinski · 2025-01-08T08:43:36Z

.github/workflows/ci-code.yml

-        from-lock: 'false'
+        from-lock: 'true'
+        # NOTE: The `verdi devel check-undesired-imports` fails if
+        # the 'tui' extra is installed.


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?

So there are two things here:

The extras parameter was added since the default has changed. So that change is necessary.

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.

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

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.

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

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

Opened #6691 to follow up on this

unkcpz

In the new uv version, this is not the case, and uv will try to find the newest version of a package for a given python version.

This is nice!

It looks good, thanks @danielholla Just a small question.

.github/actions/install-aiida-core/action.yml

unkcpz · 2025-01-08T08:45:18Z

.github/actions/install-aiida-core/action.yml


  - 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


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.

agoscinski · 2025-01-08T13:04:06Z

Thanks for the work @danielhollas this was really helpful!

unkcpz · 2025-01-08T15:32:37Z

Thanks @danielhollas I merge it to not block other PRs on going.

Update uv.lock

b28f5a1

danielhollas force-pushed the update-lock branch from 34bffbf to 8e66b24 Compare December 29, 2024 20:01

CI: Update uv

475cedb

danielhollas force-pushed the update-lock branch from 8e66b24 to 475cedb Compare December 29, 2024 20:57

danielhollas added 3 commits December 29, 2024 22:02

Run verdi tests from lock

148142a

Make verdi tests pass

79398fb

Stupid GHA

c054b5c

danielhollas commented Dec 30, 2024

View reviewed changes

This was referenced Jan 6, 2025

Bump astral-sh/setup-uv from 4 to 5 in the gha-dependencies group #6679

Closed

Ignore pytest deprecation warning for monty dependency of pymatgen #6680

Closed

danielhollas changed the title ~~WIP: Update uv.lock + CI tweaks~~ Update uv.lock + CI tweaks Jan 7, 2025

danielhollas marked this pull request as ready for review January 7, 2025 12:42

danielhollas requested review from unkcpz and agoscinski as code owners January 7, 2025 12:42

danielhollas added 3 commits January 7, 2025 13:52

Update lock

33152f9

FORCE_COLOR in test-install.yml

0394eb1

CI: Run validate-dependency-specification with python 3.11

a456400

danielhollas force-pushed the update-lock branch from 568639a to 77f1481 Compare January 7, 2025 13:00

Unset CI to workaround the pymatgen issue

86938af

danielhollas force-pushed the update-lock branch from 77f1481 to 86938af Compare January 7, 2025 13:05

Fixup VIRTUAL_ENV for python3.9

79c81a7

danielhollas changed the title ~~Update uv.lock + CI tweaks~~ Update uv.lock + CI tweaks + fix pymatgen issue Jan 7, 2025

danielhollas mentioned this pull request Jan 7, 2025

CI: Add matrix testing for both SQLite and PostgreSQL database backends #6670

Merged

danielhollas commented Jan 7, 2025

View reviewed changes

rabbull reviewed Jan 7, 2025

View reviewed changes

.github/workflows/ci-code.yml Show resolved Hide resolved

agoscinski requested changes Jan 8, 2025

View reviewed changes

unkcpz approved these changes Jan 8, 2025

View reviewed changes

agoscinski approved these changes Jan 8, 2025

View reviewed changes

unkcpz merged commit 1093026 into aiidateam:main Jan 8, 2025
22 checks passed

danielhollas deleted the update-lock branch January 8, 2025 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danielhollas commented Dec 29, 2024 •

edited

Loading

codecov bot commented Dec 29, 2024 •

edited

Loading

danielhollas commented Dec 30, 2024 •

edited

Loading

danielhollas Dec 30, 2024

danielhollas commented Jan 7, 2025

danielhollas Jan 7, 2025

agoscinski Jan 8, 2025

unkcpz Jan 8, 2025

danielhollas Jan 7, 2025

danielhollas Jan 7, 2025

danielhollas Jan 7, 2025

agoscinski left a comment

agoscinski Jan 8, 2025

agoscinski Jan 8, 2025

danielhollas Jan 8, 2025

agoscinski Jan 8, 2025

agoscinski Jan 8, 2025 •

edited

Loading

danielhollas Jan 8, 2025

agoscinski Jan 8, 2025 •

edited

Loading

danielhollas Jan 8, 2025

agoscinski Jan 8, 2025

danielhollas Jan 8, 2025

danielhollas Jan 9, 2025

unkcpz left a comment

unkcpz Jan 8, 2025

agoscinski commented Jan 8, 2025

unkcpz commented Jan 8, 2025

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

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

Conversation

danielhollas commented Dec 29, 2024 • edited Loading

codecov bot commented Dec 29, 2024 • edited Loading

Codecov Report

danielhollas commented Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

danielhollas commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agoscinski commented Jan 8, 2025

unkcpz commented Jan 8, 2025

danielhollas commented Dec 29, 2024 •

edited

Loading

codecov bot commented Dec 29, 2024 •

edited

Loading

danielhollas commented Dec 30, 2024 •

edited

Loading

agoscinski Jan 8, 2025 •

edited

Loading

agoscinski Jan 8, 2025 •

edited

Loading