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

Julia 1.7 tests are failing #2305

Closed
penelopeysm opened this issue Aug 14, 2024 · 9 comments
Closed

Julia 1.7 tests are failing #2305

penelopeysm opened this issue Aug 14, 2024 · 9 comments

Comments

@penelopeysm
Copy link
Member

penelopeysm commented Aug 14, 2024

All tests on Julia 1.7 are failing, see e.g. this run on #2304 https://github.com/TuringLang/Turing.jl/actions/runs/10378600942

I suspect it's because on Julia 1.7, ADTypes resolves to v0.2.7 (probably because some other dependency like SciMLBase specifies that), and that breaks when used with the new v1.9.1 of LogDensityProblemsAD (despite the compat entry being modified in tpapp/LogDensityProblemsAD.jl#34).

In contrast, on Julia 1.10, ADTypes resolves to v1.7.1 and all is good.

I'm not sure how Pkg handles version conflicts like these. I do find it weird that it resolved the environment to include both v1.9.1 of LogDensityProblemsAD and v0.2.7 of ADTypes, which are incompatible — intuitively, I'd have expected it to pick a lower version of LogDensityProblemsAD.

@penelopeysm
Copy link
Member Author

penelopeysm commented Aug 14, 2024

Oh, I'm guessing it's because ADTypes is a weak dependency of LogDensityProblemsAD, and Pkg 1.7 doesn't understand those as extensions were introduced in Julia 1.9.

On Julia 1.10 if you try to do ]add LogDensityProblemsAD and ]add ADTypes@0.2, Pkg correctly downgrades LogDensityProblemsAD to v1.9.0.

On the other hand, Julia 1.7 doesn't even think that ADTypes is a dependency of LogDensityProblemsAD so happily installs incompatible versions.

@mhauru
Copy link
Member

mhauru commented Aug 14, 2024

Some possibly relevant context: At some point we dropped support for ADTypes v0.2, then ran into trouble with that, and reintroduced it.

@penelopeysm
Copy link
Member Author

Ah that's very good to know. The issue isn't really with Turing, though. I think the "issue" is with LogDensityProblemsAD, because any package that uses julia<1.9 with ADTypes and LogDensityProblemsAD will run into the same issue.

Even more generally, one would expect situations like these to happen with any package that uses extensions. Surely someone has solved this or run into this before? But I couldn't find anything on GitHub issues, Discourse, or Slack.

@devmotion
Copy link
Member

I guess one problem is that LogDensityProblemsAD uses Requires to load the optional dependencies on Julia < 1.9, but does not e.g. load them unconditionally on older versions by making them proper dependencies on Julia < 1.9. Then dependencies and compat entries would be respected by Pkg also on Julia < 1.9 as expected. But it's somewhat understandable does LogDensityProblemsAD does not want to depend on all AD packages... I guess we can easily fix the current regression but the problem will come up with other updates again.

Another problem on Julia < 1.9 is that SciML basically dropped support for Julia < 1.9 in almost all packages. Which makes it quite annoying and somewhat challenging to keep support Julia < 1.9 if you depend on parts of the SciML stack. So maybe we should generally consider dropping support for Julia < 1.9 (or not depend on SciML as much by default)?

@yebai
Copy link
Member

yebai commented Aug 14, 2024

not depend on SciML as much by default

That sounds like a better long term solution. To clarify, why we depend on SciML?

@devmotion
Copy link
Member

tpapp/LogDensityProblemsAD.jl#35 should be a quick hotfix for Julia < 1.9 + ADTypes 0.2.

@penelopeysm
Copy link
Member Author

Thanks for the super quick fix @devmotion!

Separately, I've posted on Discourse, https://discourse.julialang.org/t/compat-entries-for-weakdeps-on-julia-1-9/118172 but I'm not sure that there's a nice solution to it here.

It appears to me the least bad way is for LogDensityProblemsAD to declare a requirement of julia >= 1.9, since that's the only way the compat entries will be respected. This is definitely suboptimal, because there are cases under which it could be compatible with < 1.9 and some people may well want to use that, but this is the only way to guarantee that it will work going forward. And I suppose it might be worth doing anyway based on how the Julia ecosystem is moving, though you all are the experts here.

@penelopeysm
Copy link
Member Author

Oh, I see you just said the same thing there. :)

Anyway, will rerun tests later today once the registry is updated and close this issue if they pass with the new version.

@penelopeysm
Copy link
Member Author

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

No branches or pull requests

4 participants