Build pyOpenMS from source in Windows CI pipeline #313
+98
−46
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add Python setup and pyopenms build dependencies to build-openms job - Enable PYOPENMS=ON and WITH_PARQUET=ON in CMake configuration - Add steps to build, test, and upload pyopenms wheel artifact - Download and install locally built wheel in build-executable job - Remove pyopenms from requirements.txt (built from source for all distributions) - Update README with note about installing pyopenms separately for local dev
📝 WalkthroughWalkthroughAdds pyOpenMS build/test and wheel artifact handling into the Windows executable CI; updates test CI and README to install pyopenms; removes pyopenms from requirements; renames dataframe columns in src/view.py for plotting and filtering. Changes
Sequence Diagram(s)sequenceDiagram
participant Actions as GitHub Actions Runner
participant WinBuild as Windows OpenMS Build
participant PyBuild as pyOpenMS Wheel Build & Test
participant Artifact as Artifact Storage
participant Packager as Packaging Job
rect rgba(30,144,255,0.5)
Actions->>WinBuild: Configure CMake with PYOPENMS, Python vars, WITH_PARQUET, VS2022/x64
WinBuild-->>Actions: Build OpenMS binaries
end
rect rgba(34,139,34,0.5)
Actions->>PyBuild: Set up Python, install deps, link against OpenMS
PyBuild-->>Actions: Build pyOpenMS wheel and run tests
end
rect rgba(255,165,0,0.5)
Actions->>Artifact: Upload pyOpenMS wheel
Packager->>Artifact: Download pyOpenMS wheel
Packager->>Packager: Install wheel into embeddable & final packaging Python contexts
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/build-windows-executable-app.yaml:
- Around line 186-193: The CI test step "Test pyOpenMS" is masking failures
because the pytest command is suffixed with "|| true", causing the job to always
succeed; remove the "|| true" from the pytest invocation in the "Test pyOpenMS"
step so the step fails on non-zero pytest exit codes, or alternatively replace
it with explicit exit-code handling (e.g., run pytest and capture its exit code
then exit with that code) if you need to capture output before failing — update
the shell block that runs "cd $GITHUB_WORKSPACE/OpenMS/bld/pyOpenMS" and the
pytest line accordingly.
In `@README.md`:
- Around line 31-42: The numbered steps in README.md are inconsistent: "Install
dependencies" is labeled step 2 while the next section "Launch the app" is
labeled step 4; renumber the steps to be sequential (change "Launch the app"
from 4 to 3 or adjust surrounding step numbers) so the installation section
("Install dependencies") and subsequent launch instructions ("Launch the app")
follow 2 → 3 → ... consistently throughout the file; ensure any cross-references
or TOC entries (if present) reflect the new numbering.
🧹 Nitpick comments (1)
.github/workflows/build-windows-executable-app.yaml (1)
294-298: Installation step is functional but lacks error handling.The step correctly locates and installs the wheel. However, if the wheel file is missing (e.g., build failure in previous job),
$wheelwould be$nulland the pip install would fail with an unclear error.💡 Optional: Add validation for missing wheel
- name: Install locally built pyOpenMS wheel run: | $wheel = Get-ChildItem -Path pyopenms-wheel -Filter "*.whl" | Select-Object -First 1 + if (-not $wheel) { + Write-Error "No pyOpenMS wheel found in pyopenms-wheel directory" + exit 1 + } Write-Host "Installing pyOpenMS wheel: $($wheel.FullName)" .\python-${{ env.PYTHON_VERSION }}\python -m pip install $wheel.FullName --no-warn-script-location
- Remove || true from pytest command to fail workflow on test failures - Fix step numbering in README.md (2 -> 3 for Launch the app)
Since pyopenms was removed from requirements.txt (built from source for Docker and Windows distributions), the test workflow needs to install it separately via pip, consistent with README instructions.
pyopenms 3.3.0+ requires NumPy 2.0+. The previous pin to numpy==1.26.4 caused import failures when installing the latest pyopenms.
pyopenms 3.5.0 standardized DataFrame column names to PEP 8 compliance: - 'RT' → 'rt' - 'MZ' → 'mz' (already lowercase in codebase) - 'nativeID' → 'native_id' Updated src/view.py to use lowercase 'rt' column name.
pyopenms 3.5.0 standardized DataFrame column names to PEP 8 snake_case: - 'mzarray' → 'mz_array' - 'intarray' → 'int_array' Updated src/view.py to use the new column names.
pyopenms 3.5.0 standardized DataFrame column names: - For get_df(long=False): 'intarray' → 'intensity_array' - For get_df(long=True): 'inty' → 'intensity' Updated src/view.py to use the correct column names.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/build-windows-executable-app.yaml:
- Around line 65-68: The CI step named "Install pyOpenMS build dependencies"
installs pip packages with "numpy>=1.24.0" which conflicts with
requirements.txt's "numpy>=2.0.0"; update the pip install line in that workflow
step to use "numpy>=2.0.0" (and ensure any other occurrences in the workflow
match requirements.txt) so build-time numpy matches runtime expectations.
- Around line 186-193: The CI step "Test pyOpenMS" currently pipes pytest output
to head (the pytest invocation: "pytest tests/ -v
--ignore=tests/test_tutorial.py -x -q 2>&1 | head -100"), which can hide
failures because the pipeline return code is from the last command; update the
step to enable pipefail or capture pytest's exit code: either add a shell option
(e.g., run the step with "set -o pipefail" before running pytest) or run pytest
redirecting output to a temp file and then print the head while explicitly
exiting with pytest's exit code; apply this change in the "Test pyOpenMS" run
block so failures are propagated correctly.
In @.github/workflows/workflow-tests.yml:
- Line 22: The workflow installs pyopenms without a version pin which can pull
an incompatible release; update the pip install step in the CI workflow (the
line currently installing pyopenms) to pin the version to the one used by your
code (e.g., change to pip install pyopenms==3.5.0) so src/view.py’s expected
column names (rt, mz_array, intensity_array) match the installed pyOpenMS API.
🧹 Nitpick comments (2)
requirements.txt (1)
53-53: Consider upper-bounding the numpy version.The
>=2.0.0pin allows any future numpy version. If numpy 3.x releases with breaking changes, this could cause unexpected failures. Consider using a bounded range likenumpy>=2.0.0,<3.0.0for safer dependency management..github/workflows/workflow-tests.yml (1)
13-17: Consider updating checkout and setup-python actions to v4/v5.Using
actions/checkout@v3andactions/setup-python@v3while the Windows workflow uses@v4/@v5. Newer versions include security fixes and improvements.♻️ Suggested update
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Set up Python - uses: actions/setup-python@v3 + uses: actions/setup-python@v5 with: python-version: "3.10"
Comment on lines
65
to
68
| - name: Install pyOpenMS build dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install setuptools wheel "cython>=3.0.0" "autowrap>=0.24" "numpy>=1.24.0" pandas pytest |
There was a problem hiding this comment.
Numpy version constraint inconsistent with requirements.txt.
Build dependencies specify numpy>=1.24.0 but requirements.txt requires numpy>=2.0.0. This inconsistency could lead to wheel compatibility issues if the build uses numpy 1.x while runtime expects 2.x.
🔧 Suggested fix to align versions
- pip install setuptools wheel "cython>=3.0.0" "autowrap>=0.24" "numpy>=1.24.0" pandas pytest
+ pip install setuptools wheel "cython>=3.0.0" "autowrap>=0.24" "numpy>=2.0.0" pandas pytest🤖 Prompt for AI Agents
In @.github/workflows/build-windows-executable-app.yaml around lines 65 - 68,
The CI step named "Install pyOpenMS build dependencies" installs pip packages
with "numpy>=1.24.0" which conflicts with requirements.txt's "numpy>=2.0.0";
update the pip install line in that workflow step to use "numpy>=2.0.0" (and
ensure any other occurrences in the workflow match requirements.txt) so
build-time numpy matches runtime expectations.
Comment on lines
+186
to
+193
| - name: Test pyOpenMS | ||
| shell: bash | ||
| run: | | ||
| cd $GITHUB_WORKSPACE/OpenMS/bld/pyOpenMS | ||
| pip install dist/*.whl | ||
| python -c "import pyopenms; print(f'pyOpenMS version: {pyopenms.__version__}')" | ||
| # Run basic tests (skip tutorial tests which may require additional data) | ||
| pytest tests/ -v --ignore=tests/test_tutorial.py -x -q 2>&1 | head -100 |
There was a problem hiding this comment.
Piping pytest output to head may mask test failures.
In bash, the exit code of a pipeline is the exit code of the last command by default. When pytest fails but head -100 succeeds, the step may pass despite test failures. Consider using set -o pipefail or capturing the exit code explicitly.
🔧 Suggested fix using pipefail
- name: Test pyOpenMS
shell: bash
run: |
+ set -o pipefail
cd $GITHUB_WORKSPACE/OpenMS/bld/pyOpenMS
pip install dist/*.whl
python -c "import pyopenms; print(f'pyOpenMS version: {pyopenms.__version__}')"
# Run basic tests (skip tutorial tests which may require additional data)
pytest tests/ -v --ignore=tests/test_tutorial.py -x -q 2>&1 | head -100🤖 Prompt for AI Agents
In @.github/workflows/build-windows-executable-app.yaml around lines 186 - 193,
The CI step "Test pyOpenMS" currently pipes pytest output to head (the pytest
invocation: "pytest tests/ -v --ignore=tests/test_tutorial.py -x -q 2>&1 | head
-100"), which can hide failures because the pipeline return code is from the
last command; update the step to enable pipefail or capture pytest's exit code:
either add a shell option (e.g., run the step with "set -o pipefail" before
running pytest) or run pytest redirecting output to a temp file and then print
the head while explicitly exiting with pytest's exit code; apply this change in
the "Test pyOpenMS" run block so failures are propagated correctly.
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install -r requirements.txt | ||
| pip install pyopenms |
There was a problem hiding this comment.
Pin pyopenms version to ensure compatibility with column name changes.
The src/view.py changes use pyOpenMS 3.5.0 column naming (rt, mz_array, intensity_array). Installing without a version pin may install an older version with different column names (e.g., RT, mzarray, intarray), causing test failures.
🔧 Suggested fix
- pip install pyopenms
+ pip install "pyopenms>=3.5.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pip install pyopenms | |
| pip install "pyopenms>=3.5.0" |
🤖 Prompt for AI Agents
In @.github/workflows/workflow-tests.yml at line 22, The workflow installs
pyopenms without a version pin which can pull an incompatible release; update
the pip install step in the CI workflow (the line currently installing pyopenms)
to pin the version to the one used by your code (e.g., change to pip install
pyopenms==3.5.0) so src/view.py’s expected column names (rt, mz_array,
intensity_array) match the installed pyOpenMS API.
The CMake FindPython module may need an explicit path to the Python executable in addition to Python_ROOT_DIR. This matches the pattern used in the OpenMS pyopenms-wheels workflow.
The pyopenms-wheels workflow uses Python_FIND_STRATEGY="LOCATION" to help CMake locate the correct Python installation. This tells CMake to use the location hints (Python_ROOT_DIR) first.
The official pyopenms-wheels workflow uses Visual Studio 17 2022 generator instead of Ninja. This is more reliable for building Python extensions on Windows. Changes: - CMAKE_GENERATOR: "Visual Studio 17 2022" - CMAKE_GENERATOR_PLATFORM: "x64" - Updated build flags for MSBuild
The Dockerfile uses autowrap<=0.24 for pyopenms builds. Using a consistent version constraint should help avoid compatibility issues.
Visual Studio generator creates multi-configuration builds and requires specifying which configuration to test. Added BUILD_TYPE: "Release" to the test step environment variables.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/build-windows-executable-app.yaml:
- Around line 297-302: The "Install locally built pyOpenMS wheel" step can
attempt to install when no wheel was found, producing a confusing pip error; add
an explicit guard after selecting $wheel (the result of Get-ChildItem |
Select-Object -First 1) that checks for null/empty and fails fast with a clear
message (e.g., Write-Error/Write-Host then exit 1) so the job stops with a
meaningful error instead of passing an empty path to .\python-... -m pip
install; ensure the guard references the $wheel variable used later.
♻️ Duplicate comments (2)
.github/workflows/build-windows-executable-app.yaml (2)
65-68: Keep the numpy pin aligned with runtime requirements.Build-time
numpy>=1.24.0can conflict if runtime expects numpy 2.x (ABI mismatch risk for compiled wheels). Please confirm and align if needed.
189-196: Test failures can be masked by piping tohead.Without
pipefail, a failing pytest can still yield success due tohead’s exit code.🔧 Suggested fix
- name: Test pyOpenMS shell: bash run: | + set -o pipefail cd $GITHUB_WORKSPACE/OpenMS/bld/pyOpenMS pip install dist/*.whl python -c "import pyopenms; print(f'pyOpenMS version: {pyopenms.__version__}')" # Run basic tests (skip tutorial tests which may require additional data) pytest tests/ -v --ignore=tests/test_tutorial.py -x -q 2>&1 | head -100
Comment on lines
+297
to
+302
| - name: Install locally built pyOpenMS wheel | ||
| run: | | ||
| $wheel = Get-ChildItem -Path pyopenms-wheel -Filter "*.whl" | Select-Object -First 1 | ||
| Write-Host "Installing pyOpenMS wheel: $($wheel.FullName)" | ||
| .\python-${{ env.PYTHON_VERSION }}\python -m pip install $wheel.FullName --no-warn-script-location | ||
There was a problem hiding this comment.
Fail fast when the wheel artifact is missing.
If no wheel exists, $wheel.FullName becomes empty and pip install fails with a confusing error. Consider an explicit guard.
🔧 Suggested fix
- name: Install locally built pyOpenMS wheel
run: |
$wheel = Get-ChildItem -Path pyopenms-wheel -Filter "*.whl" | Select-Object -First 1
+ if (-not $wheel) { throw "pyOpenMS wheel not found in pyopenms-wheel/" }
Write-Host "Installing pyOpenMS wheel: $($wheel.FullName)"
.\python-${{ env.PYTHON_VERSION }}\python -m pip install $wheel.FullName --no-warn-script-location🤖 Prompt for AI Agents
In @.github/workflows/build-windows-executable-app.yaml around lines 297 - 302,
The "Install locally built pyOpenMS wheel" step can attempt to install when no
wheel was found, producing a confusing pip error; add an explicit guard after
selecting $wheel (the result of Get-ChildItem | Select-Object -First 1) that
checks for null/empty and fails fast with a clear message (e.g.,
Write-Error/Write-Host then exit 1) so the job stops with a meaningful error
instead of passing an empty path to .\python-... -m pip install; ensure the
guard references the $wheel variable used later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR integrates pyOpenMS building directly into the Windows executable CI/CD pipeline, replacing the pre-built PyPI dependency with a locally compiled version. This ensures the distributed Windows executable includes a pyOpenMS wheel built with the same OpenMS version and optimized for the target environment.
Key Changes
PYOPENMS=ONto build the Python bindingsWITH_PARQUET=ONfor Parquet supportImplementation Details
Summary by CodeRabbit
Documentation
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.