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 Python library handling #2317

Merged
merged 82 commits into from
Apr 20, 2023
Merged

Update Python library handling #2317

merged 82 commits into from
Apr 20, 2023

Conversation

olupton
Copy link
Collaborator

@olupton olupton commented Apr 5, 2023

  • Make libnrnpythonX.Y unconditionally minor-version-dependent, given that we do not use the Python limited API
    • Previously we used libnrnpythonX.Y on Windows but only libnrnpythonX on macOS and Linux
    • Avoid crashes due to ABI mismatch when updating to use PyConfig API
    • Update to use PyConfig API, avoiding warnings for outdated methods that are expected(?) to be removed in Python 3.12
    • Also allows us to inherit a more python-like configuration (e.g. for input encoding), reducing differences between python and nrniv -python
  • Update logic for dynamically loading Python, as python or python3 is now more likely to not be a minor version that we compiled for.
  • Overhaul CMake handling of Python to drop copy-pasted modules and hacky clearing of CACHE variables.
    • Now we search for Python in a subprocess and only transfer exactly the information that we need back.
  • Add an extensive suite of "pyinit" tests that compare python, nrniv -python and many error cases.
    • NRN_PYTHON_EXTRA_FOR_TESTS option (list of valid Pythons that NEURON is not built against) allows explicit testing of error cases.
  • Better support for virtual environments; things like -DPYTHON_EXECUTABLE=/path/to/venv/bin/python should just work
  • nrn_find_python_module is now aware of multiple Python versions; ALL option checks if they all have a package
  • documentation updates; dropped some duplicate HOC stuff in the Python section; added a new how-to-run-scripts page
  • many nrnpy_* global variables are folded into a single neuron::python::methods struct, which holds pointers to functions that have to be built for specific versions of Python inside the libnrnpythonX.Y libraries; this makes the handling of such function pointers more consistent
    • not all of these variables have been updated; the work could be completed in a follow-up PR

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #2317 (92e8ebf) into master (400e4c8) will increase coverage by 0.04%.
The diff coverage is 78.64%.

@@            Coverage Diff             @@
##           master    #2317      +/-   ##
==========================================
+ Coverage   58.79%   58.84%   +0.04%     
==========================================
  Files         625      626       +1     
  Lines      119921   119960      +39     
==========================================
+ Hits        70513    70587      +74     
+ Misses      49408    49373      -35     
Impacted Files Coverage Δ
src/ivoc/classreg.cpp 100.00% <ø> (ø)
src/ivoc/datapath.cpp 65.38% <ø> (ø)
src/ivoc/graph.cpp 49.40% <ø> (ø)
src/ivoc/graphvec.cpp 69.69% <ø> (ø)
src/ivoc/grglyph.cpp 68.35% <ø> (ø)
src/ivoc/ivoc.cpp 81.94% <ø> (ø)
src/ivoc/ivocvect.cpp 85.08% <ø> (ø)
src/ivoc/mlinedit.cpp 3.75% <ø> (ø)
src/ivoc/ocbox.cpp 63.85% <ø> (ø)
src/ivoc/ocdeck.cpp 41.43% <ø> (ø)
... and 44 more

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 757d2f5 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ e063b3f -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@olupton olupton force-pushed the olupton/python-overhaul branch from 1cb784c to 27d38bd Compare April 13, 2023 09:31
@bbpbuildbot

This comment has been minimized.

olupton added a commit that referenced this pull request Apr 14, 2023
@olupton olupton force-pushed the olupton/python-overhaul branch from fabcd8b to f7fa518 Compare April 14, 2023 08:29
olupton added a commit that referenced this pull request Apr 14, 2023
* Improved clang-format config.
@olupton olupton force-pushed the olupton/python-overhaul branch from b8ba9dc to aba38f4 Compare April 14, 2023 09:51
@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@olupton olupton force-pushed the olupton/python-overhaul branch from d378029 to f1f0d0e Compare April 17, 2023 15:59
@bbpbuildbot

This comment has been minimized.

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 3926c5c -> Azure artifacts URL

@olupton olupton force-pushed the olupton/python-overhaul branch from f0c5055 to c414dc6 Compare April 19, 2023 06:17
@azure-pipelines
Copy link

✔️ c414dc6 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

Copy link
Member

@pramodk pramodk left a comment

Choose a reason for hiding this comment

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

I didn't read the docs & cmake parts (but I see that Robert has at least looked at the docs).

The only thing that I looked into a bit more was at src/nrniv/nrnpy.cpp as historically this is the file where I/we were stumbling on. (In case @nrnhines wants to take a look at this PR, this could be one file to review maybe?)

With the improvements in CI & testing, this is really nice! 👌

test/pyinit/CMakeLists.txt Show resolved Hide resolved
@azure-pipelines
Copy link

✔️ 464b5e1 -> Azure artifacts URL

@bbpbuildbot

This comment has been minimized.

@azure-pipelines
Copy link

✔️ 92e8ebf -> Azure artifacts URL

@olupton olupton merged commit f58692e into master Apr 20, 2023
@olupton olupton deleted the olupton/python-overhaul branch April 20, 2023 14:13
olupton added a commit that referenced this pull request Apr 27, 2023
* Revert one nrnpy_py change.
* Legacy compatibility for sys.path[0] inside nrnpython inside a HOC script
@JCGoran JCGoran mentioned this pull request Jan 16, 2024
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.

5 participants