bpart: Redesign representation of implicit imports #57755

Keno · 2025-03-13T10:14:27Z

Current Design

When we explicitly import a binding (via either using M: x or import), the corresponding bpart representation is a direct pointer to the imported binding. This is necessary, because that is the precise semantic representation of the import.

However, for implicit imports (those created via using M without explicit symbol name), the situation is a bit different. Here, there is no semantic content to the particular binding being imported. Worse, the binding is not necessarily unique. Our current semantics are essentially that we walk the entire import graph (both explicit and implicit edges) and if all reachable terminal nodes are either (i) the same binding or (ii) constant bindings with the same value, the import is allowed. In this, we are supposed to ignore cycles, although the current implementation has some trouble with this (#57638, #57699).

If the import succeeds, in the current implementation, we then record an arbitrary implicit import edge as in the BindingPartition. In essence, we are creating a spanning tree of the import graph (formed from both the implicit and explicit import edges). However, dynamic algorithms for spanning tree maintenance are complicated and we didn't implement any. As a result, it is possible for later edge additions to accidentally introduce cycles (causing #57699).

An additional problem, even if we could keep a consistent spanning tree, is that the spanning tree is not unique. In practice, this is not supposed to be observable, but it is still odd to have a non-unique representation for a particular set of imports. That said, we don't really need the spanning tree. The only place that actually uses it is which(::Module, ::Symbol) which is not performance sensitive and arguably a bad API for the above reason that the answer is ill-defined.

This PR

With all these problems, let's just get rid of the spanning tree all together - as mentioned we don't really need it. Instead, we split the PARTITION_KIND_IMPLICIT into two, one for each of the two cases (importing a global binding, or importing a particular constant value). This is actually a more efficient implementation for the common case of needing to follow a lookup - we no longer need to follow all the import edges. In exchange, more work needs to be done during binding replacement to re-scan the entire import graph. This is probably the right trade-off though, since binding replacement is already a slow path.

Fixes #57638, fixes #57699, fixes #57641, fixes #57700, fixes #57343

KristofferC · 2025-03-14T12:17:23Z

Probably a good idea to run a full PkgEval here to not jump around between local minima on the backport branch too much.

Keno · 2025-03-17T22:34:34Z

@nanosoldier runtests()

nanosoldier · 2025-03-19T09:02:24Z

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

Report summary

❗ Packages that crashed

3 packages crashed on the previous version too.

✖ Packages that failed

32 packages failed only on the current version.

Package fails to precompile: 3 packages
Package has test failures: 4 packages
Package tests unexpectedly errored: 5 packages
Networking-related issues were detected: 4 packages
Test duration exceeded the time limit: 16 packages

1410 packages failed on the previous version too.

✔ Packages that passed tests

83 packages passed tests only on the current version.

Other: 83 packages

5072 packages passed tests on the previous version too.

~ Packages that at least loaded

71 packages successfully loaded only on the current version.

Other: 71 packages

2813 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

1 packages were skipped only on the current version.

Package could not be installed: 1 packages

909 packages were skipped on the previous version too.

vtjnash · 2025-03-19T13:41:23Z

there is at least one remaining bug here

julia: /source/src/module.c:278: jl_maybe_reresolve_implicit: Assertion `bpart->restriction == resolution.binding_or_const && jl_binding_kind(bpart) == resolution.ultimate_kind' failed.

[97] signal 6 (-6): Aborted
in expression starting at /home/pkgeval/.julia/packages/Polynomials/ghqdF/src/compat.jl:17
unknown function (ip: 0x7906c1884ebc) at /lib/x86_64-linux-gnu/libc.so.6
gsignal at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
abort at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x7906c1820394) at /lib/x86_64-linux-gnu/libc.so.6
__assert_fail at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
jl_maybe_reresolve_implicit at /source/src/module.c:278

Keno · 2025-03-19T20:16:46Z

Reproducer for that failure:

module M1
	module M2
		using ..M1
		module M3
			const x = 1
			export x
		end
		using .M3
		export x
	end
	using .M2
	export x
end

Keno · 2025-03-20T00:58:23Z

@nanosoldier runtests(["ParameterisedModule", "EulerAngles", "Refactoring", "EasyCurl", "ExplicitImports", "CatmullRom", "JpegGlitcher", "TuePlots", "Surge", "KalmanFilters", "BaytesOptim", "MultiCDF", "AMLPipelineBase", "IERSConventions", "ManifoldNormal", "Miletus", "TestParticle", "ClosedLoopReachability", "HydroPowerSimulations", "MomentMatching", "Dyn3d", "DFTforge", "StochasticDelayDiffEq", "SpiDy", "CRRao", "QuantumAnnealing", "FrequencySweep", "JointSurvivalModels", "NetworkJumpProcesses", "Survey", "StratIntervals", "BloqadeGates"])

nanosoldier · 2025-03-20T10:19:48Z

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

Report summary

✖ Packages that failed

7 packages failed only on the current version.

Package fails to precompile: 3 packages
Package tests unexpectedly errored: 4 packages

2 packages failed on the previous version too.

✔ Packages that passed tests

18 packages passed tests on the previous version too.

~ Packages that at least loaded

5 packages successfully loaded on the previous version too.

KristofferC · 2025-03-20T11:46:34Z

There is only one USD definition in https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/20dcb25_vs_bbb0596/Miletus.primary.log (at https://github.com/JuliaComputing/Miletus.jl/blob/4e0e63a3728e7de842ad4a5288141815cac1922c/src/currency.jl#L82) I think? So why is there an ambiguity error?

(The same with https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/20dcb25_vs_bbb0596/AMLPipelineBase.primary.log where it is defined at https://github.com/IBM/AMLPipelineBase.jl/blob/2ae5ea4055b44d0d7732cf1323fd93edf2cf9450/src/utils.jl#L243)

vtjnash · 2025-03-20T23:41:29Z

A handful of packages were blacklisted due to the old bugs, so attempt a few of those explicitly too @nanosoldier runtests(["CompileBot", "QSymbolicsBase", "SpineOpt"])

KristofferC · 2025-03-21T18:31:49Z

The Miletus MWE is:

module M
    export S
    module C
        export S
        struct A end
        S = A() # making S const makes the error go away
    end
    using .C
end 

using .M
using .M.C
S
#= 
ERROR: UndefVarError: `S` not defined in `Main`
Hint: It looks like two or more modules export different bindings with this name, resulting in ambiguity. Try explicitly importing it from a particular module, or qualifying the name with the module it should come from.
=#

Since this only happens for non-const bindings I think it is the same issue for https://github.com/IBM/AMLPipelineBase.jl/blob/2ae5ea4055b44d0d7732cf1323fd93edf2cf9450/src/utils.jl#L243 since that one is also non-const.

# Current Design When we explicitly import a binding (via either `using M: x` or `import`), the corresponding bpart representation is a direct pointer to the imported binding. This is necessary, because that is the precise semantic representation of the import. However, for implicit imports (those created via `using M` without explicit symbol name), the situation is a bit different. Here, there is no semantic content to the particular binding being imported. Worse, the binding is not necessarily unique. Our current semantics are essentially that we walk the entire import graph (both explicit and implicit edges) and if all reachable terminal nodes are either (i) the same binding or (ii) constant bindings with the same value, the import is allowed. In this, we are supposed to ignore cycles, although the current implementation has some trouble with this (#57638, #57699). If the import succeeds, in the current implementation, we then record an arbitrary implicit import edge as in the BindingPartition. In essence, we are creating a spanning tree of the import graph (formed from both the implicit and explicit import edges). However, dynamic algorithms for spanning tree maintenance are complicated and we didn't implement any. As a result, it is possible for later edge additions to accidentally introduce cycles (causing #57699). An additional problem, even if we could keep a consistent spanning tree, is that the spanning tree is not unique. In practice, this is not supposed to be observable, but it is still odd to have a non-unique representation for a particular set of imports. That said, we don't really need the spanning tree. The only place that actually uses it is `which(::Module, ::Symbol)` which is not performance sensitive and arguably a bad API for the above reason that the answer is ill-defined. # This PR With all these problems, let's just get rid of the spanning tree all together - as mentioned we don't really need it. Instead, we split the PARTITION_KIND_IMPLICIT into two, one for each of the two cases (importing a global binding, or importing a particular constant value). This is actually a more efficient implementation for the common case of needing to follow a lookup - we no longer need to follow all the import edges. In exchange, more work needs to be done during binding replacement to re-scan the entire import graph. This is probably the right trade-off though, since binding replacement is already a slow path. Fixes #57638, #57699

Keno · 2025-03-21T21:48:46Z

@nanosoldier runtests(["ParameterisedModule", "Refactoring", "ManifoldNormal", "AMLPipelineBase", "Miletus", "ExplicitImports", "DFTforge"])

nanosoldier · 2025-03-22T09:13:18Z

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

Report summary

✖ Packages that failed

1 packages failed on the previous version too.

~ Packages that at least loaded

2 packages successfully loaded on the previous version too.

Keno · 2025-03-22T09:34:43Z

Oh wait, that was the wrong pkgeval. Oh well, I'm pretty sure I fixed it. If there's a remaining issue, I guess I'll do a follow up PR.

nanosoldier · 2025-03-22T10:13:38Z

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

Report summary

✖ Packages that failed

2 packages failed only on the current version.

Package tests unexpectedly errored: 2 packages

✔ Packages that passed tests

4 packages passed tests on the previous version too.

~ Packages that at least loaded

1 packages successfully loaded on the previous version too.

KristofferC · 2025-03-22T12:12:37Z

The error message in https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/a46c5e0_vs_bbb0596/ExplicitImports.primary.log

Constant binding was imported from multiple modules

feels quite unclear. What binding? What modules? Why is it an issue to import a constant binding from multiple modules?

When we explicitly import a binding (via either `using M: x` or `import`), the corresponding bpart representation is a direct pointer to the imported binding. This is necessary, because that is the precise semantic representation of the import. However, for implicit imports (those created via `using M` without explicit symbol name), the situation is a bit different. Here, there is no semantic content to the particular binding being imported. Worse, the binding is not necessarily unique. Our current semantics are essentially that we walk the entire import graph (both explicit and implicit edges) and if all reachable terminal nodes are either (i) the same binding or (ii) constant bindings with the same value, the import is allowed. In this, we are supposed to ignore cycles, although the current implementation has some trouble with this (#57638, #57699). If the import succeeds, in the current implementation, we then record an arbitrary implicit import edge as in the BindingPartition. In essence, we are creating a spanning tree of the import graph (formed from both the implicit and explicit import edges). However, dynamic algorithms for spanning tree maintenance are complicated and we didn't implement any. As a result, it is possible for later edge additions to accidentally introduce cycles (causing #57699). An additional problem, even if we could keep a consistent spanning tree, is that the spanning tree is not unique. In practice, this is not supposed to be observable, but it is still odd to have a non-unique representation for a particular set of imports. That said, we don't really need the spanning tree. The only place that actually uses it is `which(::Module, ::Symbol)` which is not performance sensitive and arguably a bad API for the above reason that the answer is ill-defined. With all these problems, let's just get rid of the spanning tree all together - as mentioned we don't really need it. Instead, we split the PARTITION_KIND_IMPLICIT into two, one for each of the two cases (importing a global binding, or importing a particular constant value). This is actually a more efficient implementation for the common case of needing to follow a lookup - we no longer need to follow all the import edges. In exchange, more work needs to be done during binding replacement to re-scan the entire import graph. This is probably the right trade-off though, since binding replacement is already a slow path. Fixes #57638, fixes #57699, fixes #57641, fixes #57700, fixes #57343 (cherry picked from commit 60a9f69)

Keno · 2025-03-25T18:50:23Z

This is the following case:

julia> module X1
       const x = 1
       export x
       end
Main.X1

julia> module X2
       const x = 1
       export x
       end
Main.X2

julia> using .X1, .X2

julia> which(@__MODULE__, :x)

In this particular example:

julia> using Logging

julia> which(Main, Symbol("@warn"))
ERROR: Constant binding was imported from multiple modules
Stacktrace:
 [1] binding_module
   @ ./runtime_internals.jl:181 [inlined]
 [2] which(m::Module, s::Symbol)
   @ Base ./reflection.jl:923
 [3] top-level scope
   @ REPL[2]:1

which is from this hack here: https://github.com/JuliaLang/julia/blob/master/stdlib/Logging/src/Logging.jl#L11-L30.

I'm not entirely sure what to do about this case. It used to return an arbitrary answer between the two of them. We could try to return all the modules and have ExplicitImports choose explicitly through some mechanism. Independently, We could also consider removing the hack in Logging - I don't think it is required any longer.

KristofferC · 2025-03-25T18:58:55Z

Pkg also does this

https://github.com/JuliaLang/Pkg.jl/blob/b323a38299903e613540455c044aedfab382f94f/src/Pkg.jl#L183

Keno · 2025-03-25T19:05:51Z

Independently, We could also consider removing the hack in Logging - I don't think it is required any longer.

#57891

Pkg also does this

It possibly shouldn't do that either?

I do think we may want to also tweak the behavior of which, but it's a little hard to evaluate without seeing a case where there is a compelling reason for the status quo.

Fixes a regression introduced in #57755 seen on PkgEval in #57755 (comment)

Fixes a regression introduced in #57755 seen on PkgEval in #57755 (comment) (cherry picked from commit 1d2e165)

Keno force-pushed the kf/circularimplicits branch from d7b361b to b7b70c5 Compare March 13, 2025 10:16

Keno mentioned this pull request Mar 13, 2025

Nightly fails with "ERROR: LoadError: UndefVarError: === not defined in GMT.Gdal" #57641

Closed

Keno force-pushed the kf/circularimplicits branch 3 times, most recently from a5af611 to 9f0919f Compare March 17, 2025 17:16

Keno mentioned this pull request Mar 17, 2025

add missing wb in jl_maybe_reresolve_implicit #57804

Merged

Keno force-pushed the kf/circularimplicits branch from 9f0919f to 6f0a4a4 Compare March 17, 2025 19:06

Keno added the backport 1.12 Change should be backported to release-1.12 label Mar 18, 2025

topolarity mentioned this pull request Mar 18, 2025

ELFVersion.jl additions JuliaIO/ObjectFile.jl#51

Merged

Keno force-pushed the kf/circularimplicits branch from 6f0a4a4 to 20dcb25 Compare March 20, 2025 00:57

KristofferC mentioned this pull request Mar 20, 2025

Backports release 1.12 #57536

Merged

Keno force-pushed the kf/circularimplicits branch from 20dcb25 to a46c5e0 Compare March 21, 2025 21:47

Keno merged commit 60a9f69 into master Mar 22, 2025
10 of 11 checks passed

Keno deleted the kf/circularimplicits branch March 22, 2025 09:33

KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Mar 24, 2025

Keno added a commit that referenced this pull request Mar 25, 2025

Fix corner case of double module import

4132a2a

Fixes a regression introduced in #57755 seen on PkgEval in #57755 (comment)

Keno mentioned this pull request Mar 25, 2025

Fix corner case of double module import #57892

Merged

KristofferC pushed a commit that referenced this pull request Mar 26, 2025

Fix corner case of double module import (#57892)

1d2e165

Fixes a regression introduced in #57755 seen on PkgEval in #57755 (comment)

KristofferC pushed a commit that referenced this pull request Mar 26, 2025

Fix corner case of double module import (#57892)

5ae693e

Fixes a regression introduced in #57755 seen on PkgEval in #57755 (comment) (cherry picked from commit 1d2e165)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpart: Redesign representation of implicit imports #57755

bpart: Redesign representation of implicit imports #57755

Keno commented Mar 13, 2025 •

edited

Loading

KristofferC commented Mar 14, 2025

Keno commented Mar 17, 2025

nanosoldier commented Mar 19, 2025

❗ Packages that crashed

✖ Packages that failed

✔ Packages that passed tests

~ Packages that at least loaded

➖ Packages that were skipped altogether

vtjnash commented Mar 19, 2025

Keno commented Mar 19, 2025

Keno commented Mar 20, 2025

nanosoldier commented Mar 20, 2025

✖ Packages that failed

✔ Packages that passed tests

~ Packages that at least loaded

KristofferC commented Mar 20, 2025 •

edited

Loading

vtjnash commented Mar 20, 2025

KristofferC commented Mar 21, 2025 •

edited

Loading

Keno commented Mar 21, 2025

nanosoldier commented Mar 22, 2025

✖ Packages that failed

~ Packages that at least loaded

Keno commented Mar 22, 2025

nanosoldier commented Mar 22, 2025

✖ Packages that failed

✔ Packages that passed tests

~ Packages that at least loaded

KristofferC commented Mar 22, 2025

Keno commented Mar 25, 2025

KristofferC commented Mar 25, 2025

Keno commented Mar 25, 2025

bpart: Redesign representation of implicit imports #57755

bpart: Redesign representation of implicit imports #57755

Conversation

Keno commented Mar 13, 2025 • edited Loading

Current Design

This PR

KristofferC commented Mar 14, 2025

Keno commented Mar 17, 2025

nanosoldier commented Mar 19, 2025

❗ Packages that crashed

✖ Packages that failed

✔ Packages that passed tests

~ Packages that at least loaded

➖ Packages that were skipped altogether

vtjnash commented Mar 19, 2025

Keno commented Mar 19, 2025

Keno commented Mar 20, 2025

nanosoldier commented Mar 20, 2025

✖ Packages that failed

✔ Packages that passed tests

~ Packages that at least loaded

KristofferC commented Mar 20, 2025 • edited Loading

vtjnash commented Mar 20, 2025

KristofferC commented Mar 21, 2025 • edited Loading

Keno commented Mar 21, 2025

nanosoldier commented Mar 22, 2025

✖ Packages that failed

~ Packages that at least loaded

Keno commented Mar 22, 2025

nanosoldier commented Mar 22, 2025

✖ Packages that failed

✔ Packages that passed tests

~ Packages that at least loaded

KristofferC commented Mar 22, 2025

Keno commented Mar 25, 2025

KristofferC commented Mar 25, 2025

Keno commented Mar 25, 2025

Keno commented Mar 13, 2025 •

edited

Loading

KristofferC commented Mar 20, 2025 •

edited

Loading

KristofferC commented Mar 21, 2025 •

edited

Loading