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

make extension not load twice on workers #49441

Merged
merged 2 commits into from
Apr 22, 2023
Merged

make extension not load twice on workers #49441

merged 2 commits into from
Apr 22, 2023

Conversation

KristofferC
Copy link
Member

Fixes #49437

The issue is the following:

  • Distributed wants to load packages on workers when someone loads a package on the master.
  • Therefore it inserts a package callback to load the package whenever the master loads a package.
  • Upon all the triggers for a package getting loaded on the master, this causes two copies of the extension to try to get loaded:
    • One from the callback (fired from the master)
    • And one due to all the triggers being loaded on the worker.

An alternative to this PR could be to never trigger the extension loading on workers and always let the callback handle it but I don't know how to identify a worker (myid) in Base.

@KristofferC KristofferC added packages Package management and loading backport 1.9 Change should be backported to release-1.9 labels Apr 20, 2023
@KristofferC KristofferC requested a review from vtjnash April 20, 2023 12:01
@KristofferC
Copy link
Member Author

KristofferC commented Apr 20, 2023

The alternative would look something like

 function run_extension_callbacks(pkgid::PkgId)
+    distributed_id = PkgId(UUID((0x8ba89e20_285c_5b6f, 0x9357_94700520ee1b)), "
Distributed")
+    Distributed = get(loaded_modules, distributed_id, nothing)
+    if Distributed !== nothing && isdefined(Distributed, :myid)
+        id = @invokelatest Distributed.myid()
+        # Do not trigger extensions to load on workers, they will be loaded by the callback Distributed inserts
+        id != 1 && return
+    end

@marius311
Copy link
Contributor

marius311 commented Apr 20, 2023

Thanks for this quick fix. The solution in the PR seems right to me because at least based on your description / my basic understanding (ie I havent actually tested), it will handle this correctly:

using Foo, Distributed
addprocs(2)
using Bar

which, based on current rules, should load Foo, Bar, and BarFooExt on master, and only Bar on workers. Conversely, letting master trigger the worker-loads I don't know if it could really handle that easily.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Shouldn't Base.toplevel_load[] be false already, since the extension is not the toplevel load?

@KristofferC
Copy link
Member Author

Shouldn't Base.toplevel_load[] be false already, since the extension is not the toplevel load?

I don't know the exact definition of that but adding

        @show Base.toplevel_load[]
        @show mod

to

function _require_callback(mod::Base.PkgId)
if Base.toplevel_load[] && myid() == 1 && nprocs() > 1
# broadcast top-level (e.g. from Main) import/using from node 1 (only)
@sync for p in procs()
p == 1 && continue
@async_unwrap remotecall_wait(p) do
Base.require(mod)
nothing
end
end
end
end

shows:

Base.toplevel_load[] = true
mod = BarFooExt [4863ffa8-0e90-52b0-a292-a281b01911ec]

@KristofferC
Copy link
Member Author

KristofferC commented Apr 21, 2023

I think an extension loading like this is arguably a top level load, no; it doesn't get loaded into any other package. The "extension callback" here gets run after we have loaded all the triggers and at that point we are at toplevel again calling require_prelocked.

@KristofferC
Copy link
Member Author

I'll go with this for now, if someone has a cleaner solution it can always be tweaked.

@KristofferC KristofferC merged commit b1c0eac into master Apr 22, 2023
@KristofferC KristofferC deleted the kc/ext_loading_3 branch April 22, 2023 18:37
KristofferC added a commit that referenced this pull request Apr 22, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Apr 25, 2023
Keno pushed a commit that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Julia hanging with extensions + Distributed on latest backports-release-1.9
3 participants