Skip to content
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

Allow 1.9 #34

Closed
wants to merge 1 commit into from
Closed

Allow 1.9 #34

wants to merge 1 commit into from

Conversation

avik-pal
Copy link
Member

From git history, it seems like the bump to 1.10 was accidental (at least it was associated with a Aqua testing PR)

Can we allow 1.9 if the tests pass? Currently, this is a dep of SciMLBase, so it essentially means a lot of downstream packages will have to drop 1.9 to depend on the latest SciMLBase.

@ChrisRackauckas
Copy link
Member

This would need a ton of downstream testing. It's not a trivial change to make.

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a7e0efb) 32.59% compared to head (fa55fd6) 33.14%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   32.59%   33.14%   +0.55%     
==========================================
  Files           7        7              
  Lines         181      181              
==========================================
+ Hits           59       60       +1     
+ Misses        122      121       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@avik-pal
Copy link
Member Author

Was this intentional in the first place?

01a08b0 / #19 seems to bump the julia version which is not really related to the core functionality

@ChrisRackauckas
Copy link
Member

Everything is just moving to v1.10 because it's the new LTS. And there's also an entire web of changes to have to handle. NonlinearSolve.jl and BoundaryValueDiffEq.jl already required v1.9, LinearSolve.jl had some extensive versioning support that is much easier to maintain if dropped, SciMLSensitivity.jl had tests failing on v1.10 that needed some seemingly v1.10-specific changes, RecursiveArrayTools.jl had some at least v1.9-specific behavior related to stack but the downstream tests had only gotten to work on v1.10, and so in the end we can only guarantee a good experience on v1.10 and on v1.9 most things simply block SymbolicIndexingInterface v0.3.

This update took about 2 months to complete with multiple contributors, so I don't think it's trivial to backport everything here. If someone wants to get everything back to the previous LTS (v1.6) they can do so, but I would require that the entire SciML stack is re-tested and the full set is fixed up. Otherwise it's simply better to leave it fixed at working versions and simply send back patches as required.

I don't think specifically treating v1.9 as special because technically NonlinearSolve.jl is able to do v1.9+ is a good strategy either, given that it's not an LTS and v1.10 is already out. So I think we either need to support both the old and the new LTS, i.e. v1.6 and v1.10, or just the new LTS v1.10. But again, actually making v1.6 work across the board with all of the newest versions of the interfaces would be a much larger set of work than I could capably take on, so the plan is to fix them to versions that work unless someone really wants this feature and they are willing to dedicate the time to actually make it work. And when I say actually make it work, I mean all the way down to the sensitivities since I don't want any incorrect gradients just for some vanity v1.9 support.

@avik-pal
Copy link
Member Author

Makes sense, I am bumping the versions to 1.10.

@avik-pal avik-pal closed this Jan 12, 2024
@avik-pal avik-pal deleted the ap/allow1.9 branch January 12, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants