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 ngen-build Github Action to make sure 1.x numpy is used #891

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

robertbartel
Copy link
Contributor

Updating Actions depending on numpy install to use pre-2.0 numpy version. Currently, CMake will raise an error if the found numpy version is >= 2.0.0.

Changes

  • Update "Get Numpy Python Dependency" of internal ngen-build action to make pip installed numpy<2.0.0

Notes

This is going into the pending release-0.3.0 branch. The included changes will eventually make their way into production and master as part of other steps in the release process.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Target Environment support

  • Linux (Ubuntu 22.04 Action runner)
  • MacOS (MacOS 12 Action runner)

Updating Actions depending on numpy install to use pre-2.0 numpy version
as is currently required for ngen builds.
@robertbartel robertbartel added bug Something isn't working CI/CD QA/QC labels Oct 15, 2024
@robertbartel robertbartel marked this pull request as draft October 15, 2024 16:59
@robertbartel robertbartel marked this pull request as ready for review October 15, 2024 17:02
Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

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

This doesn't stop a downstream workflow from requesting numpy>=2.0.0 via inputs.additional_python_requirements. IMO that is desirable. This seems like an appropriate place to use constraints.txt. We should be able to enforce this behavior in all downstream workflows by setting the PIP_CONSTRAINT environment variable to the constraints file.

@robertbartel
Copy link
Contributor Author

I'm pretty sure that changes to actions within a PR aren't applied when actions are run for the PR. So we should expect to still see the failures in the checks that this PR is intended to address.

Someone correct me if I'm wrong though.

Using pip constraints to ensure any downstream
`input.additional_python_requirements` that may be installed also are
prevented from installing a problematic version.
@robertbartel
Copy link
Contributor Author

This doesn't stop a downstream workflow from requesting numpy>=2.0.0 via inputs.additional_python_requirements.

Yeah, good catch @aaraney. I've modified the approach.

I wanted to use an absolute path, and for lack of a more obvious place, I put the file in the root directory. Sanity check me to make sure I'm not failing to think of a good reason we should avoid that (or just a good reason to put it somewhere else).

Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks @robertbartel

@aaraney aaraney merged commit 7fdb524 into NOAA-OWP:release-0.3.0 Oct 15, 2024
10 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working CI/CD QA/QC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants