-
Notifications
You must be signed in to change notification settings - Fork 66
Drop support for Base.tail()
#703
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
base: main
Are you sure you want to change the base?
Conversation
This causes invalidations and should not be needed anymore by the upstream ChainRules.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #703 +/- ##
==========================================
+ Coverage 93.49% 93.52% +0.03%
==========================================
Files 15 15
Lines 999 989 -10
==========================================
- Hits 934 925 -9
+ Misses 65 64 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The number of failures is a bit disturbing, but they don't seem related to this PR (and they also show up on other PRs). |
|
Sorry, I mis-clicked. IMO it would be great if we could remove these definitions. There are unfortunately preexisting test failures IIRC. I'll have a closer look tomorrow. |
|
This looks fine to me. The test errors seem unrelated, in many cases caused by Aqua tests and in particular without any reference to It would be good to fix the doctest failures though, such that we're able to build docs for the upcoming release. Can you update the doctests (see https://documenter.juliadocs.org/stable/man/doctests/#Fixing-Outdated-Doctests)? |
|
Ah yeah sure, fixed them in eb83113. BTW one of them was using |
src/tangent_types/thunks.jl
Outdated
| julia> t = @thunk(3) | ||
| Thunk(var"#4#5"()) | ||
| Thunk(var"#2#3"()) |
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.
I'd suggest
| julia> t = @thunk(3) | |
| Thunk(var"#4#5"()) | |
| Thunk(var"#2#3"()) | |
| julia> t = @thunk(3); |
or a doctest filter. But I think removing the output is simpler.
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.
Yeah good idea, fixed in c2e51fd.
|
Can you update the version number? |
|
Sure, done in 10e0ec5. |
Project.toml
Outdated
| name = "ChainRulesCore" | ||
| uuid = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4" | ||
| version = "1.26.0" | ||
| version = "1.26.1" |
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.
This is a breaking change - it will definitely break older ChainRules releases and it might break packages that define rules using Base.tail.
| version = "1.26.1" | |
| version = "2.0.0" |
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.
Ah ok, fixed in 09cb407.
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.
I also removed the compat bound in docs/Project.toml so that the docs will build. There's quite a few more failing tests now, I guess because downstream packages aren't compatible anymore.
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.
Making this a breaking change is really annoying though.
Can we see if anyone is using it?
I suspect they were not, since Base.tail isn't part of Base's public API.
Easy way to see if people are using it is to just release it with this as a nonbreaking change,
and then if people complain tag a bug fix that reverts the change.
Like it will break old ChainRules.jl releases, but only pretty old ones that there is no particular reason to be using, unless they are locked in a Manifest.toml.
But in that case the version of ChainRulesCore.jl is also going to be locked to an old version.
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.
It's annoying but it's the only way to avoid breaking existing releases, of ChainRules and possibly other packages.
SciML is making non-breaking releases that break existing versions of SciML and non-SciML packages quite regularly in my experience, and I think that's far more annoying than making a breaking release. Even if people would complain and a subsequent bug fix release would be made, there would still be a combination of package versions in the registry that is officially declared to be compatible but in fact isn't.
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.
Even if people would complain and a subsequent bug fix release would be made, there would still be a combination of package versions in the registry that is officially declared to be compatible but in fact isn't.
This is why Pkg should refuse to install things other than the latest patch release of a package (at least post 1.0.0)
But can imitate that by yanking prior ones.
I guess arguably Base.tail is part of the public API.
It wasn't when I wrote those overloads.
But it is now documented as public.
At the moment there is 283 direct reverse dependencies.
That is a lot to make do a update.
I guess dependabot will catch them though.
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.
Am I correct in thinking that all those direct dependents could use package extensions on ChainRules.jl instead?
|
(bump) |
This causes invalidations and should not be needed anymore by ChainRules.jl since the rule that uses it: #576 (comment)
Was deleted in JuliaDiff/ChainRules.jl#680.
I ran this on the ChainRules.jl tests and saw a ton of failures locally (also seen in CI), but none that seem related to this change. Disclaimer: I have almost no understanding of this kind of thing 🙈
Fixes #576.
Fixes these invalidations seen in CurveFit.jl: