8000 Build PyOpenMS with numpy2 by poshul · Pull Request #7539 · OpenMS/OpenMS · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Build PyOpenMS with numpy2 #7539

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 29, 2024
Merged

Build PyOpenMS with numpy2 #7539

merged 7 commits into from
Oct 29, 2024

Conversation

poshul
Copy link
Collaborator
@poshul poshul commented Jul 3, 2024 •
edited by github-actions bot
Loading

User description

Description

I've tested building with numpy2 instead of numpy1 and it looks like everything builds without any changes. This is intended to offer a longer term solution to #7511 .

Checklist

  • Make sure that you are listed in the AUTHORS file
  • Add relevant changes and new features to the CHANGELOG file
  • I have commented my code, particularly in hard-to-understand areas
    • 8000
  • New and existing unit tests pass locally with my changes
  • Updated or added python bindings for changed or new classes (Tick if no updates were necessary.)

How can I get additional information on failed tests during CI

Click to expand If your PR is failing you can check out
  • The details of the action statuses at the end of the PR or the "Checks" tab.
  • http://cdash.openms.de/index.php?project=OpenMS and look for your PR. Use the "Show filters" capability on the top right to search for your PR number.
    If you click in the column that lists the failed tests you will get detailed error messages.

Advanced commands (admins / reviewer only)

Click to expand
  • /reformat (experimental) applies the clang-format style changes as additional commit. Note: your branch must have a different name (e.g., yourrepo:feature/XYZ) than the receiving branch (e.g., OpenMS:develop). Otherwise, reformat fails to push.
  • setting the label "NoJenkins" will skip tests for this PR on jenkins (saves resources e.g., on edits that do not affect tests)
  • commenting with rebuild jenkins will retrigger Jenkins-based CI builds

⚠️ Note: Once you opened a PR try to minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).

PR Type

enhancement, dependencies

Description

  • Updated the numpy version to the latest in the .github/workflows/pyopenms-wheels.yml file.
  • Removed the specific version constraint (numpy<=1.26.4) and replaced it with pip install -U numpy.
  • Ensures compatibility with newer versions of numpy for building PyOpenMS.

Changes walkthrough 📝

Relevant files
Dependencies
pyopenms-wheels.yml
Update numpy version to the latest in CI workflow               

.github/workflows/pyopenms-wheels.yml

  • Updated numpy version to the latest in multiple sections.
  • Removed the specific version constraint for numpy.
    		•  +4/-4     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions github-actions bot added enhancement dependencies Pull requests that update a dependency file Review effort [1-5]: 2 labels Jul 3, 2024
    @github-actions GitHub Actions
    Copy link
    Contributor
    github-actions bot commented Jul 3, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Dependency Update:
    The PR updates the numpy version from a fixed version 'numpy<=1.26.4' to an unspecified latest version using 'pip install -U numpy'. This change could potentially introduce compatibility issues with other parts of the system that rely on specific features or behaviors of numpy. It is recommended to specify a maximum version that is known to be compatible or to perform thorough testing to ensure that the new version does not break existing functionalities.

    @github-actions GitHub Actions
    Copy link
    Contributor
    github-actions bot commented Jul 3, 2024 •
    edited by qodo-merge-pro bot
    Loading

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    ✅ Specify a version range for numpy to ensure compatibility with the project
    Suggestion Impact:The commit added a version constraint to numpy, although the version range was different from the suggestion.

    code diff:

    -          pip install -U numpy
+          pip install -U numpy >=1.23.5

    Specify a version range for numpy to ensure compatibility and prevent potential issues
    with future breaking changes in numpy. For example, use numpy>=1.26.5,<2.0.0 to allow updates
    that do not include major changes that could break compatibility.

    .github/workflows/pyopenms-wheels.yml [121]

    -pip install -U numpy
+pip install -U "numpy>=1.26.5,<2.0.0"
    Suggestion importance[1-10]: 9

    Why: Specifying a version range for numpy is a best practice to prevent potential issues with future breaking changes, ensuring compatibility and stability of the project. This suggestion is highly relevant and improves the robustness of the dependency management.

    		9

    @jpfeuffer
    Copy link
    Contributor

    I think setup.py needs to require numpy 2 as well then.

    @jpfeuffer
    Copy link
    Contributor

    Ideally setup py would require the same major version it was built with.
    Setup.py can and currently already is easily configured with cmake.

    @poshul
    Copy link
    Collaborator Author
    poshul commented Jul 3, 2024 via email

    @RalfG
    Copy link
    Contributor
    RalfG commented Oct 1, 2024

    Hi @jpfeuffer and @poshul,

    Thanks for looking in to this! I noticed the following comment:

    Ideally setup py would require the same major version it was built with.

    However, with Numpy 2, its C-API is exposed in a backwards-compatible manner. So as long as pyOpenMS is built with Numpy 2, it will work on any installation with Numpy > 1.xx.
    https://numpy.org/doc/2.1/dev/depending_on_numpy.html#numpy-2-0-specific-advice

    So the install_requires Numpy dependency in setup.py can become numpy>=1.23.5

    As an aside, are there plans for a pyOpenMS v3.2.1 soon?

    Thanks!
    Ralf

    @jpfeuffer
    Copy link
    Contributor

    Ah great, I didn't know! Then definitely let's do >=1.23.5!

    @poshul
    Copy link
    Collaborator Author
    poshul commented Oct 9, 2024

    @jpfeuffer Done!
    @RalfG If we need to we certainly can be. I want to see if the Visual C++ redist issue needs an update to the package.

    @jpfeuffer
    Copy link
    Contributor

    I think he meant to install numpy 2 (or any latest) for the build but require numpy>=1.23.5 in the setup.py

    timosachsenberg
    timosachsenberg reviewed Oct 17, 2024
    View reviewed changes
    @timosachsenberg timosachsenberg mentioned this pull request Oct 28, 2024
    Closed
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    CI Failure Feedback 🧐

    Action: build-win

    Failed stage: Download contrib build from archive (Windows) [❌]

    Failure summary:

    The action failed because the process completed with exit code 1, indicating an error occurred
    during execution. The specific cause of the error is not detailed in the provided log snippet, but
    it suggests a failure in the setup or execution of the CMake process or related commands.

    Relevant error logs: 
    1:  ##[group]Operating System
2:  Microsoft Windows Server 2022
...

179:  2123cef332c06bda05f1f114161a2d14b0a18e4a
180:  ##[group]Run jwlawson/actions-setup-cmake@v2
181:  with:
182:  cmake-version: 3.25.x
183:  github-api-token: ***
184:  use-32bit: false
185:  ##[endgroup]
186:  Using cmake version 3.25.3
187:  [command]"C:\Program Files\PowerShell\7\pwsh.exe" -NoLogo -NoProfile -NonInteractive -ExecutionPolicy Unrestricted -Command "$ErrorActionPreference = 'Stop' ; try { Add-Type -AssemblyName System.IO.Compression.ZipFile } catch { } ; try { [System.IO.Compression.ZipFile]::ExtractToDirectory('D:\a\_temp\8e3f8c43-847c-4edf-936e-c493cd9c609c', 'D:\a\_temp\63eb0427-5900-4ee1-a98b-83a362d295ce', $true) } catch { if (($_.Exception.GetType().FullName -eq 'System.Management.Automation.MethodException') -or ($_.Exception.GetType().FullName -eq 'System.Management.Automation.RuntimeException') ){ Expand-Archive -LiteralPath 'D:\a\_temp\8e3f8c43-847c-4edf-936e-c493cd9c609c' -DestinationPath 'D:\a\_temp\63eb0427-5900-4ee1-a98b-83a362d295ce' -Force } else { throw $_ } } ;"
...

457:  Python2_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.11.9\x64
458:  Python3_ROOT_DIR: C:\hostedtoolcache\windows\Python\3.11.9\x64
459:  Qt5_Dir: D:\a\OpenMS\Qt\5.12.6\msvc2017_64
460:  QT_PLUGIN_PATH: D:\a\OpenMS\Qt\5.12.6\msvc2017_64\plugins
461:  QML2_IMPORT_PATH: D:\a\OpenMS\Qt\5.12.6\msvc2017_64\qml
462:  GITHUB_TOKEN: ***
463:  ##[endgroup]
464:  no assets to download
465:  ##[error]Process completed with exit code 1.
    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    jpfeuffer
    jpfeuffer approved these changes Oct 28, 2024
    View reviewed changes
    Copy link
    Contributor
    @jpfeuffer jpfeuffer left a comment

    Choose a reason for hiding this comment

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

    Well windows is broken due to a faulty contrib release.. other than that, cannot see what could go wrong with those simple changes.

    @timosachsenberg timosachsenberg linked an issue Oct 29, 2024 that may be closed by this pull request
    Upgrade numpy in pyopenms #7634
    Closed
    @timosachsenberg timosachsenberg merged commit fd7b9f5 into develop Oct 29, 2024
    18 of 19 checks passed
    @timosachsenberg timosachsenberg deleted the Test-numpy2 branch October 29, 2024 07:52
    @hechth hechth mentioned this pull request Nov 7, 2024
    Merged
    @poshul poshul mentioned this pull request Feb 11, 2025
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @timosachsenberg timosachsenberg timosachsenberg left review comments

    @jpfeuffer jpfeuffer jpfeuffer approved these changes

    Assignees
    No one assigned
    Labels
    dependencies Pull requests that update a dependency file enhancement Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    Upgrade numpy in pyopenms
    4 participants
    @poshul @jpfeuffer @RalfG @timosachsenberg
    0