-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Fix CasADi libs workaround for macOS wheels, update to CasADi 3.6.6 for Windows wheels #4391
Fix CasADi libs workaround for macOS wheels, update to CasADi 3.6.6 for Windows wheels #4391
Conversation
Wheel builds passing: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/10601940680/job/29382886771. Opened an issue about CasADi 3.6.6 in their feedstock: conda-forge/casadi-feedstock#114. Either way, it's not needed too much since 3.6.X releases maintain minor version compatibility and don't break in the ways we have noticed in #3100. We should be fine without it even if it takes slightly more time (since we don't distribute the IDAKLU solver in our conda-forge recipe anyway). P.S. and it actually triggered a release PR, so we're good to go :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4391 +/- ##
========================================
Coverage 99.42% 99.42%
========================================
Files 292 292
Lines 22215 22215
========================================
Hits 22087 22087
Misses 128 128 ☔ View full report in Codecov by Sentry. |
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.
Changes look fine, but I won't merge this yet since it sounds like you are still waiting on Anaconda and CasADi updates
Yes, let's hope that the update gets merged soon. We should be safe in general, though (PyBaMM on conda-forge will be released only after PyPI, so we always have time to update our recipe). |
If the CasADi/Anaconda issues don't get resolved soon, then we have to revert the change in the vcpkg registry for casadi correct? |
No, we don't need to, because we can stall the PyBaMM conda-forge release during the delay. PyBaMM does not need the build-time (host) dependency on CasADi for its conda-forge recipe (because the IDAKLU solver is not compiled there yet), just the run-time dependency (which is fine with 3.6.X). |
Also, no users have reported the IDAKLU solver's absence in conda-forge to us earlier. I had earlier noted somewhere that most people seem to be installing via PyPI, and that seems to still be the case: https://anaconda.org/conda-forge/pybamm/files. Let's wait for a few days before we merge this, nevertheless. |
It is a bit off topic, but what needs to happen for IDAKLU to be included with conda-forge? If the switch to IDAKLU as the default solver ends up happening, then that is pretty critical |
Oh, that's not off-topic, I had that in mind recently – if the switch to the IDAKLU solver being the default ends up happening, then cc: @santacodes |
We can include the IDAKLU solver even right now in the PyBaMM recipe without having unvendored it – we just need to add the relevant build scripts and compile it the right way across platforms, and bump the build number (N.B., which is not the same as the version) – I've thought about it before and it isn't difficult, but it's better to do so after #4352 gets merged and becomes a part of a release (which would bring us back to the 25.1 release). |
CasADi 3.6.6 is now up on conda-forge, too: https://anaconda.org/conda-forge/casadi/files. We can now merge this safely. |
I see that the required check for auto-merge is just the RTD one, we should add the unit tests to this, at least, so that we don't auto-merge something in the future that can potentially fail. |
Description
I was going to split these into two PRs, but I decided that it would be fine to do it in the same one:
vcpkg
configuration.Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: