OpenMS icon indicating copy to clipboard operation
OpenMS copied to clipboard

Modernize pyOpenMS packaging

Open timosachsenberg opened this issue 1 month ago • 11 comments

This pull request modernizes and streamlines the Python packaging and build process for pyOpenMS, with a focus on faster dependency installation and compliance with current Python packaging standards. The main changes involve switching from pip to uv for dependency and wheel installation in CI workflows, introducing a pyproject.toml for PEP 517/518 support, and cleaning up configuration and generated files.

Build and packaging modernization:

  • Added a new pyproject.toml file to support PEP 517/518/621, specifying build requirements, metadata, dependencies, and package data for pyOpenMS.
  • Updated .github/workflows/pyopenms-wheels.yml to use uv for faster dependency and wheel installation instead of pip in all relevant build and test steps. [1] [2] [3] [4] [5] [6] [7] [8]

Configuration and code cleanup:

  • Refactored src/pyOpenMS/env.py.in to remove obsolete compilation and library variables, clarify its purpose, and focus on version/build info and Cython code generation configuration.
  • Cleaned up src/pyOpenMS/create_cpp_extension.py by removing unused imports, updating file writing to use context managers, and simplifying configuration variable usage. [1] [2] [3] [4]

Project file and artifact management:

  • Added pyproject.toml to the CMake packaging file list for inclusion in source distributions.
  • Updated .gitignore in src/pyOpenMS/pyopenms/ to exclude generated files and compiled extensions created during wheel packaging and CMake builds.- Remove debug line with incorrect libraries.extend() call in setup.py
  • Remove unused imports from setup.py (MSVS_RTLIBS, OPEN_MS_LIB, OPEN_SWATH_ALGO_LIB, PY_NUM_THREADS)
  • Remove unused imports from create_cpp_extension.py (shutil, and several env variables: OPEN_MS_CONTRIB_BUILD_DIRS, OPEN_MS_LIB, OPEN_SWATH_ALGO_LIB, OPEN_MS_BUILD_DIR, MSVS_RTLIBS, Boost_MAJOR_VERSION, Boost_MINOR_VERSION)
  • Use context managers for file operations in both scripts

Based on PR #7939 cleanup suggestions.

Summary by CodeRabbit

  • Chores

    • Modernized packaging to pyproject/PEP-517 with a CMake-driven packaging flow and platform-specific wheels.
    • Streamlined runtime version handling with resilient fallbacks and improved diagnostic output.
    • Removed legacy build-requirements and a CLI dependency-checker; added ignore rules for generated packaging/build artifacts.
  • Documentation

    • Updated build and CI instructions to use pyproject dependency groups and uv for dependency syncing; minimum Python 3.9 required.

✏️ Tip: You can customize this high-level summary in your review settings.

timosachsenberg avatar Nov 30 '25 14:11 timosachsenberg

📝 Walkthrough

Walkthrough

Switches pyOpenMS to a CMake-driven packaging flow that packages pre-built extensions, centralizes build metadata and dependencies in pyproject.toml, simplifies runtime version handling via env.py, removes legacy build-check tooling, and updates CI to install build dependencies using uv from TOML dependency groups.

Changes

Cohort / File(s) Summary
Packaging entrypoint
src/pyOpenMS/setup.py
Replace in-tree build logic with packaging of pre-built extensions; add BinaryDistribution and bdist_wheel_platform; derive/write runtime version.py; simplify setup() to rely on pyproject.toml.
CMake ⇄ env bridge
src/pyOpenMS/env.py.in, src/pyOpenMS/CMakeLists.txt
Add concise CMake→Python env variables (version, build type, src dir, threads, modules, Qt info); pre-find/cached dependencies; validate Python/NumPy/autowrap; include pyproject.toml in build output; add install targets and standalone vs in-tree handling.
PEP 621 project metadata
src/pyOpenMS/pyproject.toml
Add PEP 517/518/621 configuration: build-system, project metadata, dependency-groups (build/dev/test), and tool.py-build-cmake options for CMake build and packaging.
CI and workflows
.github/workflows/pyopenms-wheels.yml, .github/workflows/openms_ci_matrix_full.yml
Replace requirements_bld.txt installs with uv-based sync/install reading build deps from pyproject.toml; add macOS Intel/ARM jobs and standardized install/test commands across OS matrices.
Build support & cleanup
src/pyOpenMS/create_cpp_extension.py
Remove unused imports; make env import resilient with try/except and fallbacks; expand diagnostic prints; switch file writes to context-managed (with open) binary/text writes.
Removed legacy tooling
src/pyOpenMS/requirements_bld.txt, src/pyOpenMS/requirements_check.py
Remove old build requirements file and the CLI dependency-checker script.
Docs & ignore
src/pyOpenMS/README.md, src/pyOpenMS/pyopenms/.gitignore
Update README to require Python ≥3.9 and document pyproject.toml/uv usage; add ignore patterns for generated artifacts (version files, compiled extensions, generated sources).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Pay attention to src/pyOpenMS/setup.py for wheel tag handling, distclass and bdist_wheel overrides, and correctness of the runtime version.py write/read.
  • Verify env.py.in and CMake generation consistency for both in-tree and standalone modes and ensure fallback behavior in create_cpp_extension.py is safe.
  • Confirm CI uv commands resolve dependency-groups from pyproject.toml across Linux/macOS/Windows and that tool.py-build-cmake options align with CMake install targets.
  • Review CMake changes for correct handling of library copying, pyopenms install targets, generated-sentinel logic, and cleanup steps.

Possibly related PRs

  • OpenMS/OpenMS#8106 — Closely related changes to pyOpenMS packaging and CMake build/generation pipeline.
  • OpenMS/OpenMS#8111 — Overlapping CI workflow modifications; both migrate dependency installation approaches.
  • OpenMS/OpenMS#8042 — Related CI dependency-install refactor and dependency provisioning changes.

Suggested labels

Review effort 2/5

Suggested reviewers

  • timosachsenberg

Poem

🐰 I nibble TOML while CMake hums a tune,
Pre-built wheels roll out beneath the moon,
uv hops in to fetch the stack with glee,
Old scripts tucked safe beneath a memory tree,
Carrots for CI — hop forward, release we see!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Modernize pyOpenMS packaging' accurately summarizes the main objective of this comprehensive PR, which modernizes and streamlines Python packaging standards through updates to build configuration, CI workflows, and dependency management.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment
  • [ ] Commit unit tests in branch claude/complete-pr-7939-01McHnhk5pZE9C4gWxC9eEhF

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 30 '25 14:11 coderabbitai[bot]

should this still use the requirements_dev ? or all through the project toml?

timosachsenberg avatar Dec 01 '25 06:12 timosachsenberg

should this still use the requirements_dev ? or all through the project toml?

I think everything should be moved to a project toml.

pjones avatar Dec 01 '25 09:12 pjones

@timosachsenberg This is going to be a huge improvement! Thank you.

How will this affect the build documentation?

pjones avatar Dec 01 '25 09:12 pjones

But: do we need a lock file (for uv) instead? @jpfeuffer you originally suggested moving to the project toml. do we need to get rid of requirements.txt? and solely use the project toml (also for dev?)

timosachsenberg avatar Dec 01 '25 13:12 timosachsenberg

Yes, ideally you would set the [build-system] entry in the project.toml to something like https://tttapa.github.io/py-build-cmake/reference/config.html

Then, also specify build dependencies in the toml.

No requirements.txt dev/bld anymore. You can have dependency groups that can be installed separately with uv.

A bit off-topic: I think ideally we should use pixi at some point because pixi could also install compilers and c++ dependencies. Maybe not in this PR.

jpfeuffer avatar Dec 01 '25 13:12 jpfeuffer

Yeah. Right direction. But unless you add the py-build-cmake (link in previous comment but there are probably other alternatives) as a build-backend and make it call our CMake, you still need to call cmake first and then call pip build wheel --without_actually_building pyopenms. Like we do right now. You can do it in multiple PRs but I think a full rework would also fit here. The goal would be to just do uv sync or uv build and it installs python dependencies (or in the ideal future also C++ dependencies with pixi) and as build backend calls CMake. And in the end creates a wheel from the CMake outputs.

jpfeuffer avatar Dec 01 '25 17:12 jpfeuffer

I think it needs manual intervention or a new PR. We don't want ANY python script anymore. No get_requires.py, no check_py_requirements.py, ideally even no env.py and setup.py. It should be possible to do this only with configurations in a pyproject.toml.

jpfeuffer avatar Dec 02 '25 11:12 jpfeuffer

yeah got that. the check requirements is technically not needed. but I didn't figure how to get away without the get_requires AND switching to thy py-cmake-build I also wasn't sure if it is the best way (there is one by scipy build)

timosachsenberg avatar Dec 02 '25 13:12 timosachsenberg

In my opinion, checking dependencies after installing them in the same environment with a fully controlled configuration file just makes no sense. All this trash should be thrown out.

jpfeuffer avatar Dec 03 '25 16:12 jpfeuffer

I will try with the py-build-cmake

timosachsenberg avatar Dec 03 '25 17:12 timosachsenberg