Adds axom module to python interface with axom.sidre#1896
Conversation
* Renames the extension module from `pysidre` to `_sidre` so can import `axom.sidre` * Adds python package scaffolding to `src/python` * Introduces `AXOM_PYTHON_MODULE_INSTALL_PREFIX` CMake cache variable * Update pysidre usage to `import axom.sidre as pysidre` and adds a shim to warning users that importing `pysidre` is deprecated.
This allows spack environment views to place Axom's installed Python packages onto the interpreter path automatically. This commit also emits `AXOM_PYTHON_MODULE_INSTALL_PREFIX` in the generated host-config.
One can call the macro with either a COMMAND or a SOURCE file.
kennyweiss
requested review from
Arlie-Capps,
BradWhitlock,
bmhan12,
jcs15c,
nselliott,
publixsubfan,
rhornung67 and
white238
kennyweiss
added
Sidre
Misc: Fixes typos and whitespace issues
kennyweiss
force-pushed
the
feature/kweiss/improve-python-sidre
branch
from
08306f7 to
06e053d
Compare
There was a problem hiding this comment.
Question for reviewers -- especially @bmhan12 -- do you think we need this pysidre shim?
I'm not aware of anyone that's currently using this, and the minimal fix in a script that uses this is very straightforward:
- import pysidre
+ import axom.sidre as pysidre
There was a problem hiding this comment.
do you think we need this pysidre shim?
I think we can get rid of the shim.
Thinking of packages like matplotlib, where it's standard practice to just do: import matplotlib.pyplot as plt as the recommended path (https://matplotlib.org/stable/tutorials/pyplot.html).
|
|
||
| if(NANOBIND_FOUND) | ||
| nanobind_add_module(pysidre nanobind_sidre.cpp) | ||
| # Python bindings for Sidre. |
There was a problem hiding this comment.
I think most of this logic belongs in the parent directory's CMakeLists.txt,
but I held off on moving it up for now since we only currently have one python submodule (axom.sidre)
Once we have another one, we'll likely have a better sense of what the general pattern should be.
On the other hand, I'm not against refactoring it in this PR.
There was a problem hiding this comment.
Leave it for now and refactor when we have another Python module to avoid churn?
|
|
||
| # gen python helper to build directory | ||
| set(_PYEXT_DIR ${PROJECT_BINARY_DIR}/lib) | ||
| # gen python helper to build directory. |
There was a problem hiding this comment.
Similarly here -- the installation logic for Axom's python modules probably doesn't belong in src/tools/CMakeLists.txt
| # SPDX-License-Identifier: (BSD-3-Clause) | ||
|
|
||
| import pysidre | ||
| import axom.sidre as pysidre |
There was a problem hiding this comment.
To minimize the changes in this PR, I only applied
- import pysidre
+ import axom.sidre as pysidreShould we use the full axom.sidre or perhaps something else instead of pysidre in our examples/tests for sidre's python interface?
There was a problem hiding this comment.
Is there an issue with using pysidre?
| if spec.satisfies("+python"): | ||
| # Install Axom's Python package(s) so a spack environment view merges them into | ||
| # a single site-packages and `import axom.sidre` works without updating PYTHONPATH | ||
| entries.append( | ||
| cmake_cache_path( | ||
| "AXOM_PYTHON_MODULE_INSTALL_PREFIX", | ||
| spec["python"].package.platlib, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Does this PR require updating the host-configs?
| parallel_io_concepts | ||
| sidre_conduit | ||
| mfem_sidre_datacollection | ||
| python_interface |
There was a problem hiding this comment.
There was a problem hiding this comment.
3 participants
Summary
axom.sidreinstead ofpysidreAXOM_PYTHON_MODULE_INSTALL_PREFIXCMake variable for controlling where Axom's python modules get installed, and updates spack to have axomextendpython for improved access within spack environments.run_axom_with_sidrescript now allows relative paths to the Axom module in support of this.