-
Notifications
You must be signed in to change notification settings - Fork 161
Add Python interfaces based on nanobind #796
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
base: develop
Are you sure you want to change the base?
Conversation
…eature/python-nanobind
gardner48
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished a pass over the PR updates. Still need to make a pass over the docs.
| merge-multiple: true | ||
|
|
||
| - name: Upload wheels to release | ||
| uses: softprops/action-gh-release@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest we enable immutable releases in the repo, do you know if this action would work with a draft release (see the suggested workflow)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that it should work with drafts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only actions from a limited number for sources are allowed (see the error here), so we'll need to remove this job.
|
|
||
| if (cv_mem->proj_mem) { cvProjFree(&(cv_mem->proj_mem)); } | ||
|
|
||
| free(cv_mem->python); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove python from CVodeMemRec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to remove free(IDA_mem->python); in src/ida.c (and maybe remove python from IDAMemRec).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could remove it from CVode and IDA. I will do that.
gardner48
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting in on the docs
| sphinx-toolbox | ||
| sphinxcontrib-googleanalytics | ||
| sphinx-multitoc-numbering | ||
| sphinxcontrib.bibtex | ||
| git+https://github.com/LLNL/sundials.git@feature/python-nanobind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for building the docs in this PR but what should this be after this is merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sundials4py or the main/develop branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work for newly added functions in a PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to replace the entry in the requirements.txt with a pip install command in the driver script(s) that uses the cloned branch. Now, though, I am curious how this may work with RTD builds. Particularly, for features like CUDA support. RTD would need to have a build with CUDA enabled. We may need to generate the API files offline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| void bind_sunadjointcheckpointscheme_fixed(nb::module_& m); | ||
|
|
||
| void bind_sundomeigest_power(nb::module_& m); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to users to have a table like we do for Fortran e.g., we list the core but it's not clear which of the implementations of the core classes are included without searching the list of functions provided by the core module.
| status = ARKodeSetLinearSolver(ark.get(), ls, A) | ||
| assert status == ARK_SUCCESS | ||
|
|
||
| def jac_fn(t, yvec, fyvec, J, tmp1, tmp2, tmp3, _): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the underscore in the right place?
| def jac_fn(t, yvec, fyvec, J, tmp1, tmp2, tmp3, _): | |
| def jac_fn(t, yvec, fyvec, J, _, tmp1, tmp2, tmp3): |
doc/shared/Python/Usage.rst
Outdated
| # sunrealtype* lambdaR, sunrealtype* lambdaI, | ||
| # void* user_data, N_Vector temp1, | ||
| # N_Vector temp2, N_Vector temp3) | ||
| def dom_eig(t, yvec, fnvec, temp1, temp2, temp3, _): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ordering correct?
| def dom_eig(t, yvec, fnvec, temp1, temp2, temp3, _): | |
| def dom_eig(t, yvec, fnvec, _, temp1, temp2, temp3): |
|
|
||
| Introduction.rst | ||
| Usage.rst | ||
| sundials4py-core-functions.rst |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments from looking over the autogenerated docs. Not sure how easy some of these are to address (or if they need to be), so this might be something that we work on while the interface is in beta.
- The
SUNErrCodeenum is listed but I thought these values were not accessible from Python (see this comment) - Maybe some things should be excluded from the docs e.g.,
- Enumerations, since they have the same values as in C
- Structures the user shouldn't need to interact with like
class sundials4py.core._generic_N_Vector_Ops(*args, **kwargs). The addition of(*args, **kwargs)might also be confusing (I assume they are unused)
- It might be more useful to co-locate the python signatures with the C functions since they are mostly a link back to the C docs for additional details about the function and its inputs/outputs.
- The signatures are maybe more verbose than necessary e.g.,
could maybe be more like
sundials4py.core.N_VMake_Serial(vec_length: int, v_data_1d: numpy.ndarray[dtype=float64, shape=(*), order='C'], sunctx: sundials4py.core.SUNContext_) → [sundials4py.core._generic_N_Vector]or the additional details about the types could be in the body of the function documentation.N_VMake_Serial(vec_length, v_data_1d, sunctx) → [N_Vector]
| cd sundials_root_directory | ||
| python -m venv .venv # create python virtual environment | ||
| . .venv/bin/activate # activate the python virtual environment | ||
| pip install scikit-build-core[pyproject] hatchling nanobind # this is a prerequisite for the next step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To build the interface I had to do
pip install scikit-build-core[pyproject] hatchling nanobind setuptools scikit-build pybind11
|
|
||
| If you are developing new features inside of sundials, you may also need to install the Python code generator package, | ||
| `sundials4py-generate <https://github.com/sundials-codes/sundials4py-generate>`__. The steps to install this package are | ||
| found in its repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing it, but I don't see install instructions in the repo
| merge-multiple: true | ||
|
|
||
| - name: Upload wheels to release | ||
| uses: softprops/action-gh-release@v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only actions from a limited number for sources are allowed (see the error here), so we'll need to remove this job.
gardner48
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished a pass over the docs
| not the correctness of SUNDIALS itself. | ||
|
|
||
|
|
||
| User-supplied functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note about about when/why we need the StdFn version of the wrappers (see this comment).
| *jcurPtrB = std::get<1>(result); | ||
|
|
||
| return std::get<0>(result); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a section on things to note when handwriting wrappers. A few things that came up in the review:
- When a wrapper can't be autogenerated and needs to be handwritten (for example this comment)
- Using
keep_alive(see this comment) - Use
.none()when we need to allow for passing None (see this comment)
This PR adds sundials4py, which is a Python interface to SUNDIALS.
The generator code can be found here: https://github.com/sundials-codes/sundials4py-generate.
Rendered docs: https://sundials--796.org.readthedocs.build/en/796/.
Review Requests
@drreynolds:
doc/bindings/sundials4py/arkodebindings/sundials4py/idasbindings/sundials4py/kinsolbindings/sundials4py/sundomeigestbindings/sundials4py/sunlinsolbindings/sundials4py/sunmatrixbindings/sundials4py/examples@gardner48:
Everything but can skip:
_generated.hppfiles (Steven and I will review these)bindings/sundials4py/examples(Steven and Dan are both reviewing these)bindings/sundials4py/arkode(Steven and Dan both reviewing these)bindings/sundials4py/idas(Steven and Dan both reviewing these)@Steven-Roberts
_generated.hppfiles inbindings/sundials4pybindings/sundials4py/arkodebindings/sundials4py/idasbindings/sundials4py/includebindings/sundials4py/sundialsbindings/sundials4py/nvectorbindings/sundials4py/sunadaptcontrollerbindings/sundials4py/sunadjointcheckpointschemebindings/sundials4py/sunnonlinsolbindings/sundials4py/sunmemorybindings/sundials4py/examples