Closed c-randall closed 2 weeks ago
Quick review based on the description (I've not looked at the code), and I probably won't have time for a thorough review anytime soon: (1.) seems fine to me, though it should be in its own PR. (2.) the exceptions are public, so I'm not sure why you're trying to hide them. (3.) yeah, the tests need cleaning up since the refactor, this can also be its own PR. (4.) wheels are complicated for C-extensions, and need thought and design.
While I don't think uploading scikits.odes wheels with SUNDIALS pre-installed is necessarily a bad idea, we aren't numpy (and even numpy being the bottom of the stack with a relatively simple dependency tree doesn't mean the wheels aren't carefully planned and controlled). For the DAEPACK code, as it's mostly self-contained (it'll only add the Fortran runtime to a PyPI-ready wheel), I think we could do this with little issue.
SUNDIALS on the other hand has numerous options about what gets linked in (see https://sundials.readthedocs.io/en/latest/_images/sunorg1.png), some of which have additional license implications when we distribute them. Relying on conda's configuration of SUNDIALS seems like a recipe for breakage and bug reports (we've previously got bug reports that the contributed nix instructions were broken, so I'm wary of coupling ourselves to another package manager with its own timelines and plans), we should be specifying and building SUNDIALS with a specific configuration that we inform users of, and then being really loud that if this is not what you want, you should build from source. Given a significant part of SUNDIALS seems to be the different backends, I'm not sure there is a specific build that is widely useful (unlike numpy where openblas is basically the best option always), and rather than adding more barriers to a SUNDIALS build that meets people's need by requiring --no-binary
and issues where a backend doesn't work, having no wheels means that users need to look at what SUNDIALS provides and use the appropriate solver, which they should be doing anyway.
We'd not be the only project without wheels, mpi4py also lacks wheels for a similar reason, and if conda is sufficient for people's needs, I'd rather they use conda, rather than try to make PyPI be conda.
Agree with James. It's nice to have something that just works, but if you need odes-sundials, you probably are doing work that means you better understand the options of sundials in depth
Thank you for the feedback. I had not thought about this perspective before, but these are great points.
As for this pull request, I made the following changes based on your comments:
So now this pull request only includes: 1) Still exposes SUNDIALS_VERSION as a string. 2) Imports the solver modules (cvode, cvodes, ida, idas) in the parent package so they are available when the whole package is imported. 3) I left test_solvers.py in the tests folder, but it is not being called anywhere. Happy to update with more rigorous and/or different tests based on feedback.
A few more notes:
As mentioned in issue #178 the scikits-odes-sundials package can break when trying to build against incompatible versions of SUNDIALS.
I suggested that one way to address this is to start building the binary wheel files for supported python versions and operating systems and uploading those, in addition to the tarbell, on pypi. The binary distributions would include a bundled SUNDIALS version that scikits-odes-sundials had already successfully been built with. This is how other packages with C and/or Fortran dependencies (numpy, scipy, etc.) distribute their packages.
In this pull request, I included the following changes: 1) Expose
SUNDIALS_VERSION
fromsundials_version.h
as a string to the Python package. This way, users can more easily tell which version of SUNDIALS was bundled/compiled in their installed scikits-odes-sundials package. 2) I cleaned up the__init__
file for scikits-odes-sundials by moving the exceptions into a new "hidden" module. Now that scikits-odes-sundials is loaded to pypi as a standalone package, there are benefits to installing it directly rather than through scikits-odes. Namely, if you only need the SUNDIALS wrapper, there is no longer a need to setup Fortran compilers, which are required for the full scikits-odes package due to daepack. A cleaner__init__
file that imports relevant modules and "hides" exception classes makes sense for a package that can be installed independently. 3) I added a quicktest_solvers.py
function that is used to test CVODE, CVODES, IDA, and IDAS while the binary wheels files are being built. 4) I added a new workflow that can be used when releasing scikits-odes-sundials. The workflow runs through a matrix of python versions from 3.8 to 3.12 and includes all major operating systems and CPU architectures: MacOS x86-64, MacOS arm64, Windows x86-64, and Linux x86-64. The wheels files bundle official SUNDIALS releases taken from conda-forge. At the moment, this is the broadest range of operating systems and CPU architectures I can support through GitHub workflows based on which runners they have available.I tested the full workflow and the resulting release using testpypi. You can see the results of the build workflow here and the results from the tests here. If you'd like to do any more testing or see the release, you can see the testpypi page here.
I have the new workflow, called
release-sundials.yml
, set to run only when triggered manually so you will have to change that over torelease
and update the upload information to go to pypi rather than testpypi when you are ready.If you are not interested in starting to distribute the binary files, please let me know. In that case I will likely make a new pypi project under a different name that I can try to keep linked to this repo, but that includes the binaries when releases are made, rather than just the tarbell.
Some notes:
pip install --no-binary ...
. This preserves users' abilities to have full control over which SUNDIALS version they build with if they'd prefer to wrap a separate local version and not be forced into using the bundled version.