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

Update code quality configuration #555

Merged
merged 6 commits into from
Jan 20, 2025
Merged

Update code quality configuration #555

merged 6 commits into from
Jan 20, 2025

Conversation

khaeru
Copy link
Member

@khaeru khaeru commented Jan 16, 2025

  • Bump mypy, ruff versions and update config.
    • We now follow the configuration recommended in Using mypy via pre-commit python/mypy#13916, such that all files are checked with mypy, even if only a few are edited in a particular commit. This, for instance, avoids the situation that a change to type hints in one file causes typing errors in other files that are not checked.
    • IPython/Jupyter notebooks are now included in ruff formatting.
  • Update copyright year.
  • Work around Tags no longer fetch with Git v2.48.0 actions/checkout#2041.

How to review

TBD

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • Add, expand, or update documentation. N/A, formatting only
  • Update release notes.

@khaeru khaeru added enh New features & functionality ci Continuous integration labels Jan 16, 2025
@khaeru khaeru self-assigned this Jan 16, 2025
Copy link

codecov bot commented Jan 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.9%. Comparing base (3fa1191) to head (96407b5).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #555     +/-   ##
=======================================
- Coverage   98.9%   98.9%   -0.1%     
=======================================
  Files         44      44             
  Lines       4790    4790             
=======================================
- Hits        4740    4738      -2     
- Misses        50      52      +2     
Files with missing lines Coverage Δ
ixmp/backend/jdbc.py 97.2% <100.0%> (ø)
ixmp/cli.py 100.0% <ø> (ø)
ixmp/tests/backend/test_jdbc.py 100.0% <ø> (ø)
ixmp/tests/core/test_scenario.py 100.0% <100.0%> (ø)

... and 1 file with indirect coverage changes

@khaeru
Copy link
Member Author

khaeru commented Jan 16, 2025

@glatterf42 I could, optionally, expand this PR with 2 kinds of improvements to the GitHub Actions workflows that I have prototyped in genno and elsewhere:

  1. pytest.yaml: use uv to set up Python and invoke pytest and pre-commit. As advertised, this seems to be very fast.

    It doesn't require any changes to code or packaging, so it seems to be a no-regret change.

  2. publish.yaml: use uv build and uv publish instead of other tools; drop chaining to our iiasa/actions reusable workflow; use PyPI×GitHub "trusted publishing".

    However, because of upstream issues (metadata version missmatch in uv 0.5.5 astral-sh/uv#9513[BUG] tool.setuptools.license-files results in invalid metadata pypa/setuptools#4759) this requires a change in the build-backend, for which I went with hatchling as suggested in comments on those issues. This isn't a major change, especially as I only used the build backend and did not attempt to use any of the other features that hatch advertises. But I think it begins to collide with our discussion of whether to adopt hatch, pdm, poetry, or something else, where we still don't have a clear "yes" on any option. Also, uv publish is marked experimental for the time being.

    So I would maybe avoid to do this one in ixmp and the rest of the stack, for the moment.

Please let me know briefly what you think, and I'll proceed and also make parallel changes to message_ix.

@glatterf42
Copy link
Member

Thanks for this PR :)

I agree with what you say, 1. sounds good, 2. can likely wait a little. Though I have already thought about migrating our package management to uv completely myself, i.e. migrate the pyproject.toml, generate a lock file, etc. But this can also wait until we can do 2. at least, I think.

@khaeru khaeru force-pushed the enh/code-quality branch 3 times, most recently from e3803b6 to ff7ba2a Compare January 20, 2025 10:51
- ruff v1.9.0 → v1.14.1.
  - Drop custom hook entry that uses PRE_COMMIT_MYPY_VENV.
  - Use config suggested at python/mypy#13916.
- ruff v0.3.4 → v0.9.1
  - Reformat 4 files.
  - Temporarily exclude *.ipynb files covered by new version.
- Discard commented workflow contents for pandas 2.0.0 pre-release.
- Reduce scope of exclusion for ts-graphviz/setup-graphviz#630
  workaround.
- Use Python location from setup-uv instead of $pythonLocation.
- Drop workaround for pyam-iamc exclusion of Python 3.13.
- Add --durations=20 to pytest invocation.
- Drop workaround for codecov/codecov-action#1316.
@khaeru khaeru force-pushed the enh/code-quality branch 2 times, most recently from 40cfa18 to ab33b54 Compare January 20, 2025 11:56
- Drop use of "remotes"; call install.packages() directly.
- Install libpng-dev for compiling R 'png' package from source on Linux.
- Drop workaround for actions/runner-images#11137; appears no longer necessary.
@khaeru
Copy link
Member Author

khaeru commented Jan 20, 2025

Okay, now in place. Compared to this run:

  • macOS:
    • actions/setup-python: 2:49–5:33
    • astral-sh/setup-uv: 4:11–5:07
  • Linux:
    • actions/setup-python: 2:29–4:30
    • astral-sh/setup-uv: 2:38–4:08
  • Windows:
    • actions/setup-python: 13:08–15:58
    • astral-sh/setup-uv: 11:39–12:19

So only clear daylight on Windows, but by a few minutes. However there's a lot of variability in the unrelated steps. For the ubuntu-latest-py3.13 jobs, the times for the setup step + "Install the package and dependencies":

  • actions/setup-python: 11 seconds + 53 seconds.
  • astral-sh/setup-uv: 2 seconds + 6 seconds.

So that's a clear improvement.

@khaeru khaeru requested a review from glatterf42 January 20, 2025 12:22
@khaeru khaeru added this to the 3.10 milestone Jan 20, 2025
Copy link
Member

@glatterf42 glatterf42 left a comment

Choose a reason for hiding this comment

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

Thanks, looks great to me :)
Hopeful that this will stabilize our CI :)

@khaeru khaeru merged commit 58e801c into main Jan 20, 2025
21 checks passed
@khaeru khaeru deleted the enh/code-quality branch January 20, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration enh New features & functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants