Don't make Test a dependency #164
Conversation
It should only be a test dependency
|
Tests fails because Line 22 in 6087e2f |
|
I set the repo up for the new v1.10 LTS |
|
Looks like that failure is on master and unrelated to this change. |
|
Thanks @ChrisRackauckas We had some issues with extensions before, see If/once JuliaLang/julia#52132 is backported to the LTS we can move stuff to extensions. |
|
On the contrary, the problem with circular loading of extensions only occurs if a one of the transitive dependencies triggers an extension. So if the ecosystem moves to making Test a weak dependency on Julia >= 1.9 but Accessors keeps it as a dependency, then an indirect dependency on Accessors can provoke the circular loading (which apparently was reported before in #127). See e.g. JuliaMath/InverseFunctions.jl#51 (comment) and the linked issue. |
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
| @@ -11,8 +11,6 @@ InverseFunctions = "3587e190-3f89-42d0-90ee-14403ec27112" | |||
| LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | |||
| MacroTools = "1914dd2f-81c6-5fcd-8719-6d5c9610ff09" | |||
| Markdown = "d6f4376e-aef5-505a-96c1-9c027394607a" | |||
| Requires = "ae029012-a4dd-5104-9daa-d747884805df" | |||
| Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" | |||
There was a problem hiding this comment.
Test has to be added to [weakdeps], [compat], and [extras], and a corresponding section has to be added to [extensions]
| # always included for now | ||
| include("../ext/AccessorsDatesExt.jl") | ||
| include("../ext/AccessorsLinearAlgebraExt.jl") |
There was a problem hiding this comment.
This should be kept (unless you want to demote them to weak dependencies as well in this PR).
| @@ -10,7 +10,7 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| version: | |||
| - 1.6 # LTS | |||
| - '1.10' # LTS | |||
There was a problem hiding this comment.
| - '1.10' # LTS | |
| - 1.9 # Oldest supported version | |
| - '1.10' # LTS |
|
Note that the linked julia issue seems close to be fixed. So, we'll be able to enable all these extensions (TestExt, LinearAlgebraExt, DatesExt) soon anyway – less churn, only change it once. This is not like an urgent bugfix or even a new feature, just removing a few stdlib deps. Maybe, the easiest is to just do this wholesale whenever becomes possible? Otherwise it becomes a whack-a-mole game, finding and fixing problems with each stdlib weakdep whenever they occur. And some package in an environment can depend on Test for actual functionality, so it won't be a complete solution anyway (until the underlying issue is fixed). |
That doesn't help users on older Julia versions that might run into the issue with circular loading if Accessors is a (possibly quite indirect) dependency and loads Test.
It's a fix for users that see these warnings because of Accessors. It also improves loading times for anyone who's not interested in the Test-related functionality. If Accessors keeps Test as a dependency, then it invalidates all the effort that was spent in other packages to not load Test by default since it will trigger all these optional extensions. So Accessors "breaks" the ecosystem.
I assume that's not very likely with Test - in very large environments I work with there's not a single package that requires a Test dependency. |
Actually, it's already fixed in 1.11rc – just checked, it doesn't show these scary warnings. Two examples I used to check: Then, I guess, the fix may come in the next release 1.10.5 (if it was backported).
Assuming that it does get fixed on 1.10, there won't be any supported Julia version that shows these warnings. Of course, users who don't even update to patch releases risk encountering known bugs, that's always the case. If stripping some stdlibs from strong deps is so important for some, it would really be useful to ensure that the fix indeed comes in 1.10.5 that's to be released soon :) Then we can just turn all these deps into weakdeps here and elsewhere. |
On all older Julia versions (at least 1.9 - 1.10.4) Accessors with a Test dependency can trigger these warnings if it is an indirect dependency of a package that has >= 2 extensions of which one is triggered by Test. The only way to fix this on already released Julia versions is to make Test a weak dependency of Accessors, already on Julia 1.9. Regardless of these warnings, the following point is also valid on Julia >= 1.9 and not addressed by a new Julia release:
|
Afaik, Julia officially supports the latest 1.x release + LTS. As soon as 1.10.x with the fix gets released, there won't be any supported version that triggers these warnings. Don't see any issue with the warning on old unsupported versions, especially given that it's just a warning – doesn't affect the actual behavior.
Of course these stdlibs should all be made weak dependencies, I already set it up here a year ago to make excision very straightforward: Lines 19 to 22 in 6087e2f There's no reason for Accessors to depend on Dates, LinearAlgebra, Test – aside from the linked bug in Julia. As soon as it's fixed (in a couple of weeks?), all these stdlibs should be made weakdeps. |
|
I guess there's no point in arguing any further given that you seem not to be open to any of the arguments I've provided so far. I still don't see any advantage in waiting for a new Julia release - by insisting on the Test dependency, Accessors is right now counteracting the effort of the majority of the Julia ecosystem that tries to remove Test as a dependency and Accessors is right now responsible for some of these reports about warnings on Julia >= 1.9 that users see. Both problems could be fixed by this PR and are independent of any new Julia release. |
|
This change is not "free" and has risks. That's why I suggest being very careful here.
So these warnings can appear for Test as well, it's not "special"? I haven't seen them recently but can easily imagine. |
|
As explained in e.g. this Julia issue circular loading can happen whenever
Many packages have made these changes quite a while ago without knowing about potential issues. Now that it's clear what can cause them, the best solution in terms of warnings and loading times (for existing and upcoming Julia versions) is to go through your dependency stack and update other packages as well that have not made Test a weak dependency yet. That's why we updated InverseFunctions and ChangesOfVariables recently, and that's why @ChrisRackauckas opened this PR 🙂 |
If that's all, great! Somehow I missed or forgot this explanation, or thought it's only a partial description.
Does that sound right? |
|
Yes, this seems right: (jl_Sjh3WZ) pkg> why Test
Accessors → Test
(jl_Sjh3WZ) pkg> why Dates
Accessors → Dates
(jl_Sjh3WZ) pkg> why LinearAlgebra
Accessors → ConstructionBase → LinearAlgebra
Accessors → LinearAlgebraAs an additional data point: Someone reported warnings with GeoStatsBase after Test was made a weak dependency of InverseFunctions (I could reproduce these warnings). It turned out that CommonSubExpressions, a transitive dependency on GeoStatsBase, depended on Test (even though it was only used for tests). A new release of CommonSubExpressions without the Test dependency was registered today, and with this latest release I can't reproduce the warnings anymore. |
|
Another set of warnings caused by Accessors that would be resolved if Accessors would make Test a weak dependency (on Julia >= 1.9!): JuliaStats/Distributions.jl#1900 (comment) |
|
Such an outcome was easy to anticipate when some packages went with turning stdlibs into extensions despite known Julia issues with that.
Hmm, so that's not just stdlibs? What's happening there? |
|
There are no issues as long as there are no cycles (which defeats the purpose of an extension anyway - it shouldn't be triggered unconditionally). Adding a dependency on Roots would add such a cycle to Distributions since Distributions has a weak dependency on Test but Accessors, a dependency of Roots, still depends on Test.
It's the same issue I keep telling you about 😄 Distributions has a ChainRulesCore extension but Roots depends on it, so adding Roots as a dependency of Distributions will trigger the ChainRulesCore extension unconditionally which creates a circular dependency which Pkg warns about. I already opened a PR to Roots that makes ChainRulesCore a weak dependency. |
|
Since I was pinged earlier, I think we need to both get rid of direct dependencies on e.g. Test, and at the same time any circular dependencies in the package ecosystem. This PR is just one of those steps. I don't see any problem with this, and agree 100% with @devmotion |
|
It's just so fragile right now! |
Fully support you! I was the person who created extensions in Accessors in the first place, but we had to revert them. |
|
Sorry, I don't get this hypothetical. What specific packages adding dependencies can cause circular dependency problems for Accessors extensions? Isn't this a very small set, so this problem can be solved socially? Just the people commenting in this PR very likely have the social capital to get basically any needed PRs merged. Further, I'm personally maintaining quite a few packages with up to five extensions, and don't experience these problems. |
That's just wrong - the warnings in #127 were again caused by the same problem. You could have fixed them by going through the list of dependencies and ensuring that none of them (directly or indirectly) depends on any of the weak dependencies of Accessors. From the Manifest file in that issue it is clear that you had made LinearAlgebra a weak dependency of Accessors even though ConstructionBase (dependency of Accessors) depended on LinearAlgebra and you had made Test a weak dependency even though InverseFunctions (dependency of Accessors) depended on Test. |
|
If that's as easy as you say, surely it will be trivial for you to tell how to fix #171 so that it doesn't throw "circular dependency" warnings? (see test logs in CI) |
|
Can you link the line where there is a circular dependency break? I can see it |
|
Not sure how to link to CI logs, here are screenshots: |
|
ConstructionBase.jl is still causing at least one of those, why don't we just start fixing the ones we know that @devmotion already said we need to fix |
ConstructionBase is dependency-free, how can it cause these issues? |
Great, then backporting and merging the fix from 1.11 to 1.10 should be easy as well? :) See in the logs – 1.11 doesn't show those warnings. |
|
Looks like LinearAlgebra is still in |
|
Must be an old version for you then, in https://github.com/JuliaObjects/ConstructionBase.jl/blob/master/Project.toml LinearAlgebra is a weakdep. I think this version is registered just fine. |
|
But it's also a dep?? |
|
On Julia 1.9 onwards, pkg ignores entries that are in [deps] and [weakdeps]. This is to maintain backward compatibility. |
|
Ok sorry for the noise, but then is the problem not just going through all the Accessors dependencies to analyze their direct deps? Has that been done? |
|
The actual Manifest file that lists the chain of dependencies would be much more helpful than the logs of the CI run. If I checked correctly, none of the remaining dependencies of Accessors depends on LinearAlgebra anymore (at least not in the latest versions - but many users can't update to the latest ConstructionBase version, including myself, which is why I'm advocating for doing the LinearAlgebra change in a separate PR - if at all). It's pure speculation but I assume that the problem is that some of the weak dependencies (such as AxisKeys) actually depend on LinearAlgebra. I imagine this can also cause cycles when trying to load these weak dependencies since AxisKeys will trigger the weak extension which loads both AxisKeys and Accessors but at the same time it will also trigger (through the indirect dependency on LinearAlgebra) the weak extension for LinearAlgebra which itself then tries to load both LinearAlgebra and Accessors. |
|
Maybe one could help Pkg to figure out the desired loading order by including LinearAlgebra in the list of triggers for the weak extension for AxisKeys? |
|
Right so at least a LinearAlgabraExt PR to AxisKeys.jl is needed. Then probably others. It is showing that having extensions on mid level packages like AxisKeys in a low level packages like Accessors leads to the low level one owning some of the others maintenance work (And might just not be possible if AxisKeys actually needed LinearAlgebra) Or maybe your load order trick will work, I missed that comment |
That's the main reason I'm asking to be careful in switching deps -> weakdeps! Even when given a concrete situation, it's not clear what the underlying reason for these warnings is. It's really brittle as your comments indicate. For example, Unitful depends on Dates, maybe it causes the warnings? |
It's speculation because I did not have access to the Manifest file and did not have time to investigate it in more detail myself.
There's a single problem: cycles. So no package/extension is safe per se, you just have to check (or be told by Pkg) that you do (not) create a cycle by moving something to an extension.
As pointed out previously, I think it's a design flaw to have these extensions in Accessors. IMO Accessors should have only few dependencies and also only few weak dependencies, given that it's not a leaf package but rather used by many packages in the ecosystem. My feeling is also that such dependency cycles would be less likely if e.g. the Unitful extension would be an Accessors extension in Unitful instead (and similarly e.g. the AxisKeys extension). |
|
So UnitfulDatesExt would be another necessary PR. True, this will start to get annoying. |
|
@devmotion I agree on limiting extensions here, except for very low level packages like Dates and LinearAlgebra. With Unitful Accessors could just limit itself to using the ConstructionBase compatibility I added to Unitful.jl |
Interesting take! As I saw it, Accessors is an opinionated way to "mutate immutables", while Unitful is the package to represent units in Julia. In this perspective, it makes sense to put the extension here. Still, the behavior shouldn't depend on this choice. It's unfortunate that "commenters with social capital" don't use that capital to promote fixing/backporting the existing fix to 1.10.
So, the behavior patterns with these stdlib extensions are nontrivial, as I said above multiple times. And the safest approach is to keep them as deps while the Julia bug is not fixed. As we see, this doesn't mesh well with the "run fast and break things" attitude of some ecosystems. I'm personally a bit tired with these discussions... Maybe let's risk merging @devmotion's PR, understanding that if actual reports on Accessors-caused warnings come we revert it? |
|
superceded by #168 |
It should only be a test dependency