-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Move Distributed to stdlib #24443
Move Distributed to stdlib #24443
Conversation
Bump, would be great to land this soon. I want to excise |
1d3c097
to
f94345e
Compare
On it. The build appears to be failing on CI while it is goes through successfully on my local machine. Can anyone familiar with the CI build process take a look at the failures and help me out? |
Locally reproducible with
|
6603295
to
492bb4b
Compare
@vchuravy , that is a different issue. Thanks for spotting it. I am seeing an error with
|
I am also seeing:
|
492bb4b
to
bb76a99
Compare
Need help fixing these errors with
Other functions exported from |
11057af
to
1661e00
Compare
Fixed. Ready for review. I guess we can have a single NEWS entry for all excised modules. A large number of files have been touched. Would like a few more eyes than usual going over this PR. |
@@ -0,0 +1,47 @@ | |||
|
|||
# Multi-Threading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multi-threading.md
should remain part of the base docs for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still in base. Original file has been split into three - tasks, distributed and threading, with only distributed moved into stdlib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path of the file is doc/src/stdlib/multi-threading.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an existing path for standard library doc entries. Excised docs go into the module path at stdlib/Distributed/docs/src/
. Contents of doc/src/stdlib/parallel.md
have been split into
doc/src/stdlib/parallel.md
- task API
doc/src/stdlib/multi-threading.md
- threading API
stdlib/Distributed/docs/src/index.md
which is copied into doc/src/stdlib/distributed.md
- in doc/make.jl
This is the way other excised modules have been handling docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) my mistake then.
Still some conflicts before merging. |
201da85
to
e3c89ad
Compare
Travis linux is green. I guess Travis OSX and AV have been broken for sometime now? |
AV is fixed (just restarted your job). Travis should be re-run once #24766 is green and merged. We shouldn't be merging PRs while those two are offline. |
@vtjnash can you also review this PR? |
65b5fdc
to
2baebf5
Compare
@amitmurthy Is there any specific part that needs review? Are the windows failures related? They don't appear to be. |
Also needs to be rebased. |
@ViralBShah , @vtjnash specifically |
base/deprecated.jl
Outdated
@@ -1272,6 +1272,42 @@ export conv, conv2, deconv, filt, filt!, xcorr | |||
@deprecate_moved PollingFileWatcher "FileWatching" true true | |||
@deprecate_moved watch_file "FileWatching" true true | |||
@deprecate_moved FileMonitor "FileWatching" true true | |||
# FIXME : This causes problems with the `myid` in loading.jl | |||
#@deprecate_binding Distributed nothing true ", run `using Distributed` instead" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #24843 for a potential solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried that but was causing AV build to fail for some reason. Have left a commented line in sysimg.jl for now.
@@ -276,6 +276,9 @@ end | |||
# require always works in Main scope and loads files from node 1 | |||
const toplevel_load = Ref(true) | |||
|
|||
myid() = isdefined(Main, :Distributed) ? invokelatest(Main.Distributed.myid) : 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am worried about the performance overhead of invokelatest
here.
It might be faster to define myid() = 1
here and upon loading Distributed
do a @eval Base myid() = Distributed.myid()
.
or we could define:
@eval Base begin
const _Distributed = Base.root_module(:Distributed)
myid() = isdefined(Main, :Distributed) ? _Distributed.myid() : 1
end
at the end of sysimage.jl
cc @vtjnash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have left this as is for now. The first option results in a build time warning followed by failures in the compile
test. The second one runs into dependency issues with require
in sysimg.jl
I'll open an issue to optimize these lines once this PR is merged.
Bump. Would be nice to get this excision done by the Dec 15 feature freeze. |
I'll rebase and merge it once it passes CI. Can optimize post 15th. |
a461a71
to
6bf6be4
Compare
6bf6be4
to
5b0ba79
Compare
AV and Travis(Linux) are green. Travis(OSX) is the spawn test failing. Will merge in a few hours if there are no objections. |
This PR moves Distributed to stdlib.
-p
,--machinefile
and--worker
now implicitly loadDistributed
myid
andnprocs
continue to be defined in Base to enable other libraries to test if they are running in distributed mode or notDistributed
continues to be part of sysimg.jl, however it is not part of Base. I assume this is required for the case where the julia binary distribution is installed on all machines? How willstdlib
be handled in the binary distribution case? We can removeDistributed
from sysimg.jl if stdlib is guaranteed to be available on all nodes in the cluster.