Fix missing dependencies in notebooks #43
Conversation
This includes: - scipy - seaborn - coolprop - scikits.odes
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Naikless
force-pushed
the
fix-missing-dependencies-in-notebooks
branch
from
8e2637c
to
abc5b2c
Compare
| @@ -2,7 +2,13 @@ name: cantera-latest | |||
| channels: | |||
| - cantera | |||
| - cantera/label/dev | |||
| - conda-forge | |||
There was a problem hiding this comment.
I appreciate the intention of this change. However, adding conda-forge will (I believe) result in some packages like NumPy and SciPy coming from that channel rather than the defaults. In that case, there will be an incompatibility in the compiled clib code between Cantera/Python/NumPy. There is a Cantera package on conda-forge, but we don't publish pre-releases there so I don't think that'll work for this purpose. I'm honestly not sure how to continue from here 😦
There was a problem hiding this comment.
I don't think any of the Jupyter examples require the development version of Cantera at this point, so we could just install everything from conda-forge, dropping the cantera and cantera/label/dev channels entirely.
There was a problem hiding this comment.
That seems reasonable to me, since the Jupyter examples probably should only contain stuff that is available in the stable release without requiring the user to install the dev version if they want to reproduce the example on their own machine. I'll change environment.yaml accordingly.
As a side note/question: how serious are these incompatibilities in practice? Because I would assume mixing channels by installing cantera from the cantera channel and everything else from conda-forge is a relatively common thing to do for people that don't pay much attention to this issue (which, until today, included myself).
There was a problem hiding this comment.
how serious
In the worst case, I believe they can lead to segmentation faults in the memory which cause an immediate crash of Python. More likely would be that some symbols are named differently or are placed in a different location in the binary, which would also cause errors. Here are some docs from conda-forge about it: https://conda-forge.org/docs/user/tipsandtricks.html#using-multiple-channels
Using only conda-forge as channel to avoid incompatibilities between compiled libraries.
|
@Naikless Thanks for updating the environment file! Can you re-run all the Notebooks with Cantera 3.0 installed from conda-forge and commit the result? |
|
Ah, I should have known there is a reason why this broke in the first place: |
|
Maybe in relation to this: I noticed that the conda-forge version of Cantera has these rather strict requirements regarding the sundials version (and other dependencies) whereas the version from the cantera channel does not. Also, when compiling cantera 3.0 from source, the default sundials version is 5.3. Can you elaborate on the reasons for these discrepancies? |
I think this is a preference of conda-forge because we have to compile once for every version of SUNDIALS we want to support (if I understand correctly) and that would be a lot of CI time/resources
I think we can pick our own choice here, and it's also possible we have it mis-specified.
We only update this infrequently, as needed. This is a version that is stable and well tested so we feel good about keeping it as the default for source builds. |
|
Thanks for the details. Would it be possible to add a Cantera 3.0 Version to the conda-forge channel that works with |
I'm not sure. Can you open an issue at https://github.com/conda-forge/cantera-feedstock? If you can find any other examples of feedstocks that support multiple versions of SUNDIALS that'd be helpful to know how to support this. |
|
To keep things relatively simple, I'd suggest just ignoring the inability to run the example that's making use of |
I just did. However, if this is a waste of resources, we can also just follow @speth suggestion, although cleaning up disfunctional examples was what initially motivated this PR. 😬 |
Naikless
force-pushed
the
fix-missing-dependencies-in-notebooks
branch
from
da705de
to
13fd0e7
Compare
- add missing dependency for ipywidgets - removal of scikits.odes from environment.yaml due to sundials dependency conflict. This prevents the following examples from running: - 1D_packbed.ipynb - 1D_pfr_surfchem.ipynb
|
@speth What do you think here? It is possible to rebuild and support older SUNDIALS versions, but it N times the CI jobs for conda-forge. I suspect since this example is out-dated, we should probably just delete it anyways? |
I don't think that's consistent with how conda-forge is meant to work, is it? I thought the idea was that all dependent packages should be rebuilt to support new versions of a dependency (I suppose with the core Python package being the exception), so the place where this should be resolved is in https://github.com/conda-forge/scikits.odes-feedstock. Otherwise, the number of build combinations required grows unwieldy with more than one dependency. I'd say let's keep this as simple as possible. The work I'm doing now on Cantera/enhancements#178 will eventually eliminate this repo as a standalone entity, though any requirements to run these examples will become requirements for a full build of the documentation website, so it is important to understand any difficulties in satisfying those dependencies. |
Yes, that's my understanding as well, except that in this case I believe support for SUNDIALS >6.6 needs to be added upstream in scikits.odes first.
Just to be clear, you're saying that this PR should be merged as-is and we'll handle the outdated dependency in your work for Cantera/enhancements#178? |
Yes, though I haven't looked at the re-rendered output here myself. |
Many of the jupyter examples are currently broken due to missing dependencies.
This should at least fix some of them.