Change Distributed.cluster_manager from a global non-constant ClusterManager to a global constant Ref{ClusterManager}

[DilumAluthge]

No description provided.

[DilumAluthge]

@JamesWrigley It looks like this isn't sufficient to fix the JET error. Example CI log:

═════ 2 possible errors found ═════
┌ start_worker(cookie::AbstractString; kwargs...) @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/cluster.jl:241
│┌ start_worker(out::IO, cookie::AbstractString) @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/cluster.jl:242
││┌ start_worker(out::IO, cookie::AbstractString; close_stdin::Bool, stderr_to_stdout::Bool) @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/cluster.jl:260
│││┌ Task(f::Distributed.var"#26#27") @ Base ./task.jl:5
││││┌ (::Distributed.var"#26#27")() @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/cluster.jl:262
│││││┌ process_messages(r_stream::Base.PipeEndpoint, w_stream::Base.PipeEndpoint, incoming::Bool) @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/process_messages.jl:151
││││││┌ Task(f::Distributed.var"#process_messages##2#process_messages##3"{Base.PipeEndpoint, Base.PipeEndpoint, Bool}) @ Base ./task.jl:5
│││││││┌ (::Distributed.var"#process_messages##2#process_messages##3"{Base.PipeEndpoint, Base.PipeEndpoint, Bool})() @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/process_messages.jl:151
││││││││┌ message_handler_loop(r_stream::Base.PipeEndpoint, w_stream::Base.PipeEndpoint, incoming::Bool) @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/process_messages.jl:164
│││││││││┌  @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/process_messages.jl:319
││││││││││ `Distributed.cluster_manager` may be undefined: Distributed.cluster_manager
│││││││││└────────────────────
│││││││││┌  @ Distributed /home/runner/work/Distributed.jl/Distributed.jl/src/process_messages.jl:332
││││││││││ `Distributed.cluster_manager` may be undefined: Distributed.cluster_manager
│││││││││└────────────────────

Test Summary: | Fail  Total   Time
JET           |    1      1  11.0s

[DilumAluthge]

That being said, this might still be a good change to make, because it makes it a bit more explicit that the global exists, and isa ClusterManager.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.38%. Comparing base (6649a94) to head (d533b94).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
+ Coverage   79.34%   79.38%   +0.04%     
==========================================
  Files          10       10              
  Lines        1951     1955       +4     
==========================================
+ Hits         1548     1552       +4     
  Misses        403      403              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[aviatesk]

JET reports whenever a global variable that might be uninitialized is used.
For local variables, as you mentioned on Slack, you can avoid reporting by adding statements like @assert @isdefined(target_var) "assertion to help the compiler and JET". However, this approach does not work for global variables due to various reasons. The two options to consider are:

1. Review the code architecture and eliminate uninitialized global variable references: In this case, passing cluster_manager as an argument to each subroutine is a better approach, leading to a more functional and good coding pattern.
2. Always initialize the global variable: If the above approach is not feasible due to issues like API compatibility, you can use Ref or similar containers to always initialize the global variable, avoiding the problem. For example:

const cluster_manager = Ref{ClusterManager}()

function subroutine()
    isassigned(cluster_manager) || error("cluster manager uninitialized")
    manager = cluster_manager[]
    # use `manager`
end

This approach does not fundamentally solve the code design issue but exploits JET's permissive reporting approach though.

[DilumAluthge]

@JamesWrigley What do you think? Should we go with the Ref approach?

[JamesWrigley]

Sorry for the late review 😬 LGTM!

[JamesWrigley]

I think this is still an improvement over the previous code.

[DilumAluthge]

@JamesWrigley Do you think this is a breaking change? We've changed the type of Distributed.cluster_manager from ClusterManager to Ref{ClusterManager}, and users would now need to do Distributed.cluster_manager[] instead of Distributed.cluster_manager.

Is Distributed.cluster_manager part of the public API?

[JamesWrigley]

I'm pretty sure it isn't 🤔 It's not in the docs at least. @vchuravy, @jpsamaroo what do you think?

[DilumAluthge]

We can revert if @vchuravy or @jpsamaroo think this is breaking.