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

Compare Python & NumPy versions from configure/build time to runtime #558

Merged
merged 4 commits into from
Jul 14, 2023

Conversation

PhilMiller
Copy link
Contributor

@PhilMiller PhilMiller commented Jul 3, 2023

Fixes #545

Additions

  • Add src/core/NGen_Python_Build_Versions.in] to have CMake generate a corresponding .cpp file with the Python/NumPy versions

Changes

  • Get versions of Python and NumPy seen at runtime
  • Check runtime versions against configure/build-time versions

Testing

  1. Tests fail if I pip install numpy==1.24.0 in my virtual environment after building with 1.25.0 installed

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 ➡️

@PhilMiller
Copy link
Contributor Author

On reading a bit further into this, it will maybe be more straightforward to just do the full-up string comparisons, rather than component-wise comparisons.

  • Python: platform.python_version()
  • NumPy: np.version.full_version

@PhilMiller PhilMiller force-pushed the 545-python-version branch from 8ac83d7 to b281804 Compare July 11, 2023 22:09
@PhilMiller PhilMiller marked this pull request as ready for review July 11, 2023 22:09
endfunction()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file's change was something I noticed as I was implementing the main goal of this PR

@@ -29,7 +29,6 @@ target_link_libraries(realizations_catchment PUBLIC
)

if(NGEN_ACTIVATE_PYTHON)
target_include_directories(realizations_catchment PUBLIC ${PROJECT_SOURCE_DIR}/extern/pybind11/include)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was something I noticed as I was implementing the main goal of this PR

@PhilMiller
Copy link
Contributor Author

The PR template checklist says this

Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)

Are we actually doing this? This PR has been noted as something that should be communicated to users. I just don't see anything at all in the change log

@donaldwj
Copy link
Contributor

I see how this make the compile time python version available but where is the runtime check? I would have expected modification to Ngen.cpp where the runtime check is made.

@mattw-nws
Copy link
Contributor

I'm assuming @donaldwj found the runtime checks he referred to, but yes, for future reference, they're here:
https://github.com/NOAA-OWP/ngen/pull/558/files#diff-3605ac7d3b4e8fb163eb3ee233ee2573740249d9961efc54675c0a571196dd4eR102-R108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Python and NumPy version checks
3 participants