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

Implement an interface for Distributed-like libraries #871

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

Conversation

JamesWrigley
Copy link
Contributor

This moves support for Distributed into a package extension and adds an interface for other distributed worker libraries (i.e. DistributedNext.jl) to use to support Revise for their workers. Admittedly the interface is quite... clunky... so I'm open to other ideas. This also required bumping the minimum Julia version to 1.9 to use package extensions.

As a bonus the load times are slightly improved:

# v3.6.4
julia> @time_imports using Revise
      2.7 ms  OrderedCollections
      0.2 ms  UUIDs
      3.8 ms  CodeTracking
               ┌ 0.2 ms JuliaInterpreter.__init__() 
    136.9 ms  JuliaInterpreter 72.95% compilation time
     35.6 ms  LoweredCodeUtils
      0.7 ms  Serialization
               ┌ 0.0 ms Distributed.__init__() 
     31.7 ms  Distributed 71.09% compilation time
               ┌ 0.0 ms NetworkOptions.__init__() 
      2.2 ms  NetworkOptions
      0.4 ms  Printf
               ┌ 0.2 ms MbedTLS_jll.__init__() 
      2.4 ms  MbedTLS_jll
               ┌ 0.1 ms LibSSH2_jll.__init__() 
      2.1 ms  LibSSH2_jll
               ┌ 0.1 ms LibGit2_jll.__init__() 
      2.0 ms  LibGit2_jll
      7.5 ms  LibGit2
               ┌ 0.1 ms Revise.__init__() 
     79.2 ms  Revise

# This PR
julia> @time_imports using Revise
      3.3 ms  OrderedCollections
      3.4 ms  CodeTracking
               ┌ 0.2 ms JuliaInterpreter.__init__() 
    145.1 ms  JuliaInterpreter 73.80% compilation time
     36.3 ms  LoweredCodeUtils
               ┌ 0.1 ms Revise.__init__() 
     80.9 ms  Revise

Copy link
Owner

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I'm strongly supportive of anything that reduces dependency on Distributed, especially if the replacements are less vulnerable to invalidations. The overall design of this PR seems OK.

Project.toml Outdated
JuliaInterpreter = "0.9"
LoweredCodeUtils = "3.0.1"
OrderedCollections = "1"
# Exclude Requires-1.1.0 - see https://github.com/JuliaPackaging/Requires.jl/issues/94
Requires = "~1.0, ^1.1.1"
julia = "1.6"
julia = "1.9"
Copy link
Owner

Choose a reason for hiding this comment

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

I'd say let's go straight to 1.10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that, fixed in b468b35.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized there's some cleanup we can do now that 1.10 is required so I've split that out into a separate commit in 67ee77c.


# Wrapper struct to indicate a worker belonging to the Distributed stdlib. Other
# libraries should make their own types for Revise to dispatch on.
struct DistributedWorker
Copy link
Owner

Choose a reason for hiding this comment

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

Does this have to be here or could it be in the extension? I guess it's here because of init_worker(p::Int) below?

Also, should we define an AbstractWorker and then suggest people subtype? I'm not sure it helps concretely, but it seems mildly tidier to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah exactly it's because of the init_worker(::Int) definition. Technically I think that could also go in the extension, but I slightly lean towards keeping it in the main package since the extension doesn't really 'own' the Int type that we're dispatching on.

Agreed about the AbstractWorker type, added it in b468b35.

Mostly so that we can use package extensions.
This moves support for Distributed into a package extension and adds an
interface for other distributed worker libraries to use to support Revise for
their workers.
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.

2 participants