-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Add testing with new versions of our vendored dependencies #758
Comments
For that matter, the same comment applies to Eigen (although we haven't had problems with breaking changes there), fmt, and yaml-cpp. |
Now that SUNDIALS has an official Git repo (https://github.com/LLNL/sundials), we can discontinue our unofficial mirror. That should make updating the submodule version less onerous. From what I can tell, most of the changes with the newer SUNDIALS versions are new features that we don't currently have a way of using, so there's not really much immediate benefit for use within Cantera. The case that we do need to support, though, is users who want to use their own copy of SUNDIALS because they are linking Cantera with their own code that also uses SUNDIALS. |
That's fine, I don't think we need to update the submodule all that frequently. We should have CI tests that run against multiple versions though, IMO, so we can catch breaking changes before we get error reports. Hopefully, we can make it easy to add new versions of dependencies like this. Now that I think of it, we should do this for all our major dependencies (for some definition of major). Any ideas where that boundary is? I realize this will add more overhead in the CI suite, I'm hoping using something like Azure Pipelines (with its 10 builds in parallel) will help here. Let's make a wish list of dependencies we'd like to test multiple versions of and see what we can do about that? |
The experience I've had with SUNDIALS has been that every major version (and some minor versions) has introduced API changes requiring changes on our end, so it may be a bit of a special case compared to other dependencies. I think the issues have always been visible at compile time, so they're pretty obvious, and once they're resolved they tend to stay that way, given the abstraction of all SUNDIALS use into the I agree the issue is broader than the list of dependencies which we have submodule version of -- those are in some ways the simpler case, since we can always insist on using those versions if a newer or older version of something is problematic. Certainly we could afford a few more CI builds if we moved to Azure Pipelines, as you've advocated before. I see that as less of a limitation than the developer time it takes to set up new testing environments, especially when dealing with things that aren't nicely packaged (although I see that there are now conda packages for SUNDIALS, so maybe things aren't so bad). |
This is related to #775, which implements checking of multiple versions of SUNDIALS. |
Since we do test for multiple versions of Sundials, as well as fmt, yaml-cpp and Eigen (either from |
My vision for the resolution of this issue is to move checks with multiple versions of dependencies into a separate workflow that only runs on pushes to main. This would also allow us to run tests against the main branches of our dependencies and hopefully catch and resolve failures early without totally blowing up test times on PRs |
I think the original issue described has been resolved, and the remaining ideas for further improvement are better tracked/discussed under the umbrella of Cantera/enhancements#37. |
LLNL is now up to version 5.0.0 of SUNDIALS, as of October 2019: https://computing.llnl.gov/projects/sundials/release-history
In our submodule, we're still on 3.1.0. We should come up with a way of keeping up to date with SUNDIALS releases, since it seems like they're pushing new code much more frequently now than a few years ago. This is especially important for anyone using a system version of SUNDIALS, since we've had versions of SUNDIALS change their API when upgrading previously.
The text was updated successfully, but these errors were encountered: