Skip to content

changed: bump ForwardDiff compat in ControlSystems + ControlSystemsBase also #978

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

Closed
wants to merge 2 commits into from

Conversation

franckgaga
Copy link
Member

@franckgaga franckgaga commented Mar 30, 2025

more complete than #977

Some tests do not pass locally, but I think it was already the case before. (edit: just tested locally and yes, they were failing also before)

BTW, I can also try to do #975 myself.

@franckgaga franckgaga changed the title changed: bump ForwardDiff in ControlSystems + ControlSystemsBase also changed: bump ForwardDiff compat in ControlSystems + ControlSystemsBase also Mar 30, 2025
Copy link

codecov bot commented Mar 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.41%. Comparing base (5ab74cf) to head (9e00f30).
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #978   +/-   ##
=======================================
  Coverage   92.41%   92.41%           
=======================================
  Files          41       41           
  Lines        4981     4981           
=======================================
  Hits         4603     4603           
  Misses        378      378           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@franckgaga
Copy link
Member Author

franckgaga commented Mar 30, 2025

Weird it does not pass locally but it passes here. Oh well, good news XP

@baggepinnen
Copy link
Member

baggepinnen commented Mar 31, 2025

Did you verify that the tests actually ran with the new version? OrdinaryDiffEq might not yet be compatible so the tests might have run with an old version.

BTW, I'm in Jordan 🇯🇴 on holiday with very spotty internet connections, so my response times may be long 😊

@franckgaga
Copy link
Member Author

franckgaga commented Apr 3, 2025

Forgot to answer but yes you are right the test are executed with v0.10 instead of v1.0. I'm starting to understand why the CompatHelper forces the upper bound of compat entries.

But at the same time, I find hard to fully understand dependency issues. If everyone (as in every package) is waiting for everyone to update the ForwardDiff compat entries in their Project.toml because of this issue, it feels like nobody will update it. Except if we are "lucky" enough and everything is done in the correct order, for exemple:

  1. A dependancy of OrdinaryDiffEq.jl update it's codebase to be compatible with ForwardDiff v1.0 and update the compat entry
  2. OrdinaryDiffEq.jl update it's codebase to be compatible with ForwardDiff v1.0 and update the compat entry
  3. ControlSystemsBase.jl update it's codebase to be compatible with ForwardDiff v1.0 and update the compat entry
  4. ModelPredictiveControl.jl update it's codebase to be compatible with ForwardDiff v1.0 and update the compat entry (I decided to update it anyway, FYI)

And realistically, there is probably many steps before step 1. Am I misunderstanding something if I feel like "waiting for the other to update in the correct order" is almost a Mission: Impossible?

edit: I will post my question on discourse, I'm probably not the only one who does not understand this part.

edit2: Chris answered my quesiton here

edit3: Forgot to wish you a wonderful trip to Jordan, have fun as much as you can 🇯🇴 😄 !!!

@franckgaga
Copy link
Member Author

I will close this PR since now you have two separate CompatHelper PRs. BTW, OrdinaryDiffEq.jl now supports ForwardDiff v1.0, so you may try to re-run the jobs in #977 and it may passes now.

@franckgaga franckgaga closed this Apr 19, 2025
@baggepinnen
Copy link
Member

BTW, OrdinaryDiffEq.jl now supports ForwardDiff v1.0

Where did you get that from? It does not look like it here

https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/Project.toml#L89

@franckgaga
Copy link
Member Author

Hum weird, my mistake. I look at the same place yesterday and I saw "0.10, 1". I'm hallucinating like a LLM 😆

@baggepinnen
Copy link
Member

You might have looked another package with a similar name, there are very many :P

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