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.

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

Increase CI sensitivity by testing both lowest-direct and highest dependency resolution strategy #640

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

janosh
Copy link
Member

@janosh janosh commented Jul 4, 2024

related PRs: materialsproject/pymatgen#3852 + CederGroupHub/chgnet#159

the benefit of this kind of CI setup is that it ensures the full range of allowed version ranges in pyproject.toml actually work. e.g. resolution strategy highest could have failed since the latest versions of jobflow dependencies weren't tested before

@janosh janosh added dependencies tests Test all the things ci Continuous integration labels Jul 4, 2024
Copy link

codecov bot commented Jul 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.09%. Comparing base (a740c6c) to head (057c2d4).
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #640      +/-   ##
==========================================
- Coverage   99.23%   91.09%   -8.14%     
==========================================
  Files          21       21              
  Lines        1573     1573              
  Branches      427      427              
==========================================
- Hits         1561     1433     -128     
- Misses         10      136     +126     
- Partials        2        4       +2     
Files with missing lines Coverage Δ
src/jobflow/core/maker.py 100.00% <100.00%> (ø)

... and 4 files with indirect coverage changes

@janosh
Copy link
Member Author

janosh commented Jul 4, 2024

the py 3.12 CI run is failing due to a pyzmq build error

error: Failed to prepare distributions
  Caused by: Failed to fetch wheel: pyzmq==24.0.1
  Caused by: Failed to build: `pyzmq==24.0.1`
  Caused by: Build backend failed to build wheel through `build_wheel()` with exit status: 1

based on

$ pip show pyzmq              
Name: pyzmq
Version: 26.0.3
Location: /Users/janosh/.venv/py312/lib/python3.12/site-packages
Requires:
Required-by: ipykernel, jupyter-client, jupyter-server, maggma, notebook

looks like jobflow depends on pyzmq via maggma. tried to increase min maggma version to 0.69 which didn't help. @tschaume @tsmathis could we increase the min pyzmq version here to 25.1.1 which added 3.12 support

@utf
Copy link
Member

utf commented Jul 15, 2024

Hi @janosh, thanks for this. I'm a bit confused about what's happening here. If you use resolution strategy="highest" and the strict optional dependencies at the same time, won't this just use the pinned versions in strict? I think testing the pinned versions in strict is useful btw, but happy to add additional dependency tests on top of this.

@tschaume
Copy link
Member

@janosh I can't tell from your error message what exactly the build error is but if pyzmq==25.1.1 still supports python 3.9, it should be ok to increase the minimum version in maggma. Do the jupyter packages that depend on pyzmq enforce a specific minimum version on pyzmq?

@janosh
Copy link
Member Author

janosh commented Jul 20, 2024

If you use resolution strategy="highest" and the strict optional dependencies at the same time, won't this just use the pinned versions in strict?

@utf that's right and hence be equivalent to CI as it runs prior to this PR. the 3-fold matrix strategy does the following

- { python: "3.9", resolution: highest, extras: "tests,strict" }  # this is equivalent to the previous CI run before this PR
- { python: "3.10", resolution: lowest-direct, extras: "tests" }  # test oldest allowed versions
- { python: "3.11", resolution: highest, extras: tests }  # test latest released versions

though i just noticed a typo in the middle case (which incorrectly used strict)

@tschaume doesn't look like it, definitely not not ipykernel, nor notebook. should i submit a PR or will you bump pyzmq in maggma?

@janosh
Copy link
Member Author

janosh commented Jul 20, 2024

@utf looks like the new CI is working. it found jobflow is incompatible with the lowest pydantic>=2.0.1 it listed, needs at least 2.4 😄

@janosh
Copy link
Member Author

janosh commented Jul 20, 2024

only failing CI job now is code coverage which supposedly fell by 8%. makes no sense since no new code was added in this PR...

@tschaume
Copy link
Member

should i submit a PR or will you bump pyzmq in maggma?

@janosh feel free to go ahead and submit a PR with maggma. @Andrew-S-Rosen and @rkingsbury might have some feedback and would be able to merge your PR.

@janosh
Copy link
Member Author

janosh commented Sep 18, 2024

@utf i think this is ready to go except that the coverage seems to be thrown off by the use of pytest.importorskip on several optional dependencies which are only installed in the 1st of these 3 CI runs which uses the strict deps set. in the other 2 CI runs, tests requiring optional deps are skipped (i.e. tests that would fail if you just run pip install jobflow in an empty environment)

- { python: "3.9", resolution: highest, extras: "tests,strict" }  # this is equivalent to the previous CI run before this PR
- { python: "3.10", resolution: lowest-direct, extras: "tests" }  # test oldest allowed versions
- { python: "3.11", resolution: highest, extras: tests }  # test latest released versions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration dependencies tests Test all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants