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

prevent loading other extensions when precompiling an extension #55589

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Aug 26, 2024

The current way of loading extensions when precompiling an extension very easily leads to cycles. For example, if you have more than one extension and you happen to transitively depend on the triggers of one of your extensions you will immediately hit a cycle where the extensions will try to load each other indefinitely. This is an issue because you cannot directly influence your transitive dependency graph so from this p.o.v the current system of loading extension is "unsound".

The test added here checks this scenario and we can now precompile and load it without any warnings or issues.

Would have made #55517 a non issue.

Fixes #55557

The current way of loading extensions when precompiling an extension very easily leads to cycles. For example, if you have more than one extension and you happen to transitively depend on the triggers of one of your extensions you will immidiately hit a cycle.

The test added here checks this scenario and we can now precompile and load it without any warnings or issues.
@KristofferC KristofferC added the needs pkgeval Tests for all registered packages should be run with this change label Aug 26, 2024
@KristofferC
Copy link
Sponsor Member Author

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@KristofferC
Copy link
Sponsor Member Author

@dlfivefifty, you seem to be the only one using this feature (at least in PkgEval) in e.g. https://github.com/JuliaArrays/LazyArrays.jl/blob/7777232394f9e42a7685cbf15001bf4b3bcba696/ext/LazyArraysBlockBandedMatricesExt.jl#L25.

To check, would it be possible and how hard would it be to provide the same functionality without requiring this. The reason is that extensions loading extensions (as it is done now) is quite brittle and easily lead to cycles and bad experience. There might be a slightly more "relaxed" version of this that could be implemented that could maybe still allow the use case in LazyArrays but I want to check with you if the simple thing could be done first.

@dlfivefifty
Copy link
Contributor

I felt this was a bit dodgy when I did it. But I don’t know how else to do this?

note this usage should never lead to cycles since BlockBandedMatrices.jl itself depends on BandedMatrices.jl

@dlfivefifty
Copy link
Contributor

Put another way: if package B depends on A, then it’s natural that an extension for B depends on an extension on A

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Sep 1, 2024

note this usage should never lead to cycles since BlockBandedMatrices.jl itself depends on BandedMatrices.jl

If you happen to get two of your triggers for your extensions (say BlockBandedMatrices and StaticArrays) into your full dependency graph of LazyArrays you get a cycle because loading LazyArrays will trigger one extension and loading LazyArrays in that will trigger the other (over and over).

You cannot control your full dependency graph so this can happen at any time.

@dlfivefifty
Copy link
Contributor

I don't understand your example. What's the alternative solution?

It might be possible I can move functions/types into LazyArrays.jl in this case. But it seems like a major flaw in extensions combining with packages.

@jishnub
Copy link
Contributor

jishnub commented Sep 3, 2024

I think at present, extensions are only for adding methods to pre-existing functions from one package for pre-existing types from another. In this case, perhaps both the type LazyBandedLayout and function _mulbanded_copyto! need to me moved to LazyArrays, and only methods need to be added in the extension modules.

@KristofferC
Copy link
Sponsor Member Author

But it seems like a major flaw in extensions combining with packages

Well yeah, that's what we are trying to fix heh.

@dlfivefifty
Copy link
Contributor

dlfivefifty commented Sep 3, 2024

I still don’t understand the issue being solved. A directed tree doesn’t form cycles….

@KristofferC
Copy link
Sponsor Member Author

#52511 (comment) is one example in the wild.

You can also just setup the test example I provided in this PR and see how precompilation fails for example.

@dlfivefifty
Copy link
Contributor

I think I need a mathematical description to be convinced it’s not an implementational issue.

Packages with (or without) extensions are simply a tree. How does a cycle come up?

I how are extensions different to before when we could make packages to implement extra behaviour? Ie how is LazyArrays making extensions for BandedMatrices and BlockBandedMatrixes in any way different to making two packages called LazyArraysBandedMatrices and LazyArraysBlockBandedMatrices that depend on each other exactly like the extensions do??

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 4, 2024

Mainly because the edges point the unexpected direction from BandedMatrices and LazyArrays, since loading either of those can trigger loading the extension, so in the graph, they are the originators of the edge, rather than the terminators. That can cause a cycle to appear in the load graph that isn't as easily anticipated.

@topolarity
Copy link
Member

topolarity commented Sep 5, 2024

I've created JuliaArrays/LazyArrays.jl#345 to avoid the inter-ext dependency

This kind of workaround isn't always possible though - This wouldn't work if the types I moved (e.g. AbstractLazyBandedLayout) had fields/supertypes from both BandedMatrices.jl (the trigger) and LazyArrays.jl

I think we need to make the change that Kristoffer described here: #48734 (comment) and give this situation a definite load-/dependency-ordering, so that it's not a "cycle" any more - With that change, I think the only "cycle" left is for extensions that share the exact same trigger/parent set (e.g. extBA and extAB) for which case this fix probably makes sense

@topolarity
Copy link
Member

OK, I think we probably want to add an exception to this to still allow loading of other extensions if their triggers are a strict subset of your own (e.g. {A,C} ⊊ {A,B,C})

That'd leave room for us to do something like #49891 to allow ext -> ext dependencies, like the LazyArrays.jl case.

@dlfivefifty
Copy link
Contributor

Could a simple solution be that if an extensionA depends on extensionB it needs to list it in the Project.toml? I.e.

https://github.com/JuliaArrays/LazyArrays.jl/blob/d3e51078551d98ba951525931c77b2f259bfea29/Project.toml#L25

Would become

LazyArraysBlockBandedMatricesExt = ["LazyArraysBandedMatricesExt", "BlockBandedMatrices"]

base/precompilation.jl Outdated Show resolved Hide resolved
@KristofferC KristofferC removed the needs pkgeval Tests for all registered packages should be run with this change label Sep 27, 2024
Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @KristofferC !

This doesn't give us a solution for direct ext → ext dependencies, but we can add that piece in later.

For now, this is much better behavior 👍

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.

Extension cycles are problematic (long precompile times, difficult to understand/resolve)
6 participants