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

1.11.1: breaking change in extension loading #56204

Open
aplavin opened this issue Oct 17, 2024 · 12 comments
Open

1.11.1: breaking change in extension loading #56204

aplavin opened this issue Oct 17, 2024 · 12 comments

Comments

@aplavin
Copy link
Contributor

aplavin commented Oct 17, 2024

Not too often I see packages broken by patch versions of Julia, but it's one of those cases :(
As a specific example, using UnionCollections, Dictionaries, FlexiMaps throws ERROR: LoadError: type Nothing has no field UnionDictionary on this line: https://github.com/JuliaAPlavin/UnionCollections.jl/blob/0000000018ecd297a2e7448671da83c035b3da4c/ext/FlexiMapsDictionariesExt.jl#L8.

Effectively, it does get_extension(MyPackage, :WeakDepA) in the extension for WeakDepA + WeakDepB.
Judging by the documentation, it should work:

  • get_extension returns the extension if it's loaded

get_extension(parent::Module, extension::Symbol)
Return the module for extension of parent or return nothing if the extension is not loaded.

  • Pkg docs say the following about extension loading:

A package extension is a module in a file (similar to a package) that is automatically loaded when some other set of packages are loaded into the Julia session.

In the specific case with UnionCollections, this is what I expect to happen on that using line:

  1. UnionCollections is loaded
  2. Dictionaries is loaded
  3. UnionCollections -> DictionariesExt is loaded (following Pkg docs)
  4. FlexiMaps is loaded
  5. UnionCollections -> FlexiMapsDictionariesExt is loaded, it sees DictionariesExt loaded already and successfully imports that (following get_extension docs)

And that's indeed how the observable behavior worked on earlier Julia versions, including 1.11.0. But not on 1.11.1.

@aplavin
Copy link
Contributor Author

aplavin commented Oct 17, 2024

Juliahub search indicates that quite a few packages call get_extension in their extensions, so maybe this bug manifests itself somewhere else as well. Or maybe not, depends on the order I guess, didn't check myself.

@bvdmitri
Copy link
Contributor

Judging by the documentation, it should work:

AFAIK get_extension is internal (its also written in its docstring) and is subject to change and its use is really discouraged since the order load is not really defined.

@KristofferC
Copy link
Member

Yeah, this is a bit unfortunate. The issue was that loading extensions in other extensions as was done previously was "unsound" and could easily cause "circular dependencies" leading to bad behavior like:

I didn't see any new regressions from PkgEval from backporting it which is why I assumed it wouldn't have a big impact on the ecosystem but maybe this was hidden by other errors. In hindsight it was probably wrong to backport it.

The use case people want seem to basically be #48734 so we should probably work on fixing that feature. In the mean time the question is if we revert the extension loading change for 1.11.2.

@aplavin
Copy link
Contributor Author

aplavin commented Oct 17, 2024

AFAIK get_extension is internal (its also written in its docstring) and is subject to change

It's mentioned and explained in Pkg docs, https://pkgdocs.julialang.org/v1/creating-packages/. Afaiu, in 1.10 and earlier this means this function is public.

As for loading, the same Pkg docs page writes that

A package extension is a module in a file (similar to a package) that is automatically loaded when some other set of packages are loaded into the Julia session.

Naturally, after loading MyPkg + WeakDepA I expect MyPkg -> WeakDepAExt to be loaded as well.
And actually, seems like it does work – just not during precompilation.

So, I found a workaround: add __precompile__(false) to the extension that does get_extension (@lkdvos – may be relevant for you as well). Don't see any downsides...

I didn't see any new regressions from PkgEval from backporting it which is why I assumed it wouldn't have a big impact on the ecosystem but maybe this was hidden by other errors.

Unfortunately, yet another case of PkgEval not catching an actual issue :( Have to I say, I don't have a good understanding about it in practice. At least UnionCollections tests (example from the first post) tests succeed on 1.10 and 1.11.0, but fail on 1.11.1.

@KristofferC
Copy link
Member

I looked in the PkgEval log and UnionCollections is here: https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/0de7b6c_vs_501a4f2/UnionCollections.primary.log. And we can see the extension fails to load but the package tests end up passing anyway...

@aplavin
Copy link
Contributor Author

aplavin commented Oct 17, 2024

Indeed, you are right, I wasn't looking carefully enough! Still not 100% used to seeing "big scary errors" that are practically just warnings :)

@KristofferC
Copy link
Member

I guess there is no test that checks that the function in the extension works?

@aplavin
Copy link
Contributor Author

aplavin commented Oct 17, 2024

Indeed, maybe it slipped through the cracks over time... Anyway, I confirm that with __precompile__(false) everything loads fine and functions work afterwards.

@aplavin
Copy link
Contributor Author

aplavin commented Oct 17, 2024

Although, in some scenarios it leads to

│  ┌ Error: Error during loading of extension FlexiMapsDictionariesExt of UnionCollections, use `Base.retry_load_extensions()` to retry.
│  │   exception =
│  │    1-element ExceptionStack:
│  │    Error when precompiling module, potentially caused by a __precompile__(false) declaration in the module.

...

@topolarity
Copy link
Member

topolarity commented Oct 17, 2024

You might try using a guarded eval instead of __precompile__(false):

# workaround for https://github.com/JuliaLang/julia/issues/56204
function __init__()
    if !Base.generating_output()
        Base.eval(quote
            const DictionariesExt = Base.get_extension(UnionCollections, :DictionariesExt)
            FlexiMaps.mapview(f, d::DictionariesExt.UnionDictionary) = @modify(vs -> mapview(f, vs), DictionariesExt._values(d))

            # TODO disambiguation:
            # FlexiMaps.mapview(p::Union{Symbol,Int,String}, A::UnionDictionary) = mapview(PropertyLens(p), A)
        end)
    end
end

__precompile__(false) assumes that any downstream packages rely on this functionality, so it disables pre-compilation for them too (it's extremely viral). I also think you might have found a bug in it, where it's not being properly applied for extensions.

Using eval in a !Base.generating_output() guard like this has a similar effect of making FlexiMaps.mapview(f, d::DictionariesExt.UnionDictionary) unavailable during pre-compilation, but it avoids disabling pre-compilation for other packages which is generally preferable.

@topolarity
Copy link
Member

I think the above workaround works with an appropriate pre-compile guard (the !Base.generating_output())

@andreyz4k
Copy link

Another affected package is Transformers.js which relies on Flux's CUDA extension in its own CUDA extension. https://github.com/chengchingwen/Transformers.jl/blob/master/ext/TransformersCUDAExt/TransformersCUDAExt.jl

It compiles just fine but crashes in runtime with ERROR: type Nothing has no field check_use_cuda

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

5 participants