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

Move Distributed to stdlib #24443

Merged
merged 2 commits into from
Dec 14, 2017
Merged

Move Distributed to stdlib #24443

merged 2 commits into from
Dec 14, 2017

Conversation

amitmurthy
Copy link
Contributor

@amitmurthy amitmurthy commented Nov 2, 2017

This PR moves Distributed to stdlib.

  • Command line args -p, --machinefile and --worker now implicitly load Distributed
  • myid and nprocs continue to be defined in Base to enable other libraries to test if they are running in distributed mode or not
  • Distributed 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 will stdlib be handled in the binary distribution case? We can remove Distributed from sysimg.jl if stdlib is guaranteed to be available on all nodes in the cluster.

@JeffBezanson JeffBezanson added excision Removal of code from Base or the repository stdlib Julia's standard library labels Nov 2, 2017
@vchuravy
Copy link
Member

Bump, would be great to land this soon. I want to excise Threads and there is a dependency chain to DistributedArrays.

@amitmurthy amitmurthy force-pushed the amitm/del_dist branch 3 times, most recently from 1d3c097 to f94345e Compare November 22, 2017 08:33
@amitmurthy amitmurthy changed the title WIP: Move Distributed to stdlib Move Distributed to stdlib Nov 22, 2017
@amitmurthy
Copy link
Contributor Author

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?

@vchuravy
Copy link
Member

Locally reproducible with make -C test channels

diff --git a/test/channels.jl b/test/channels.jl
index 2d094d1531..805731b198 100644
--- a/test/channels.jl
+++ b/test/channels.jl
@@ -84,6 +84,7 @@ let s, c = Channel(32)
 end
 
 # Tests for channels bound to tasks.
+using Distributed
 for N in [0,10]
     # Normal exit of task
     c=Channel(N)
@@ -144,7 +145,6 @@ for N in [0,10]
     @test ref[] == nth
 
     # channeled_tasks
-    using Distributed
     for T in [Any, Int]
         chnls, tasks = Base.channeled_tasks(2, (c1,c2)->(assert(take!(c1)==1); put!(c2,2)); ctypes=[T,T], csizes=[N,N])
         put!(chnls[1], 1)

@amitmurthy
Copy link
Contributor Author

@vchuravy , that is a different issue. Thanks for spotting it. I am seeing an error with make docs which is what I think is causing early termination of the CI build.

WARNING: parse(str::AbstractString; kwargs...) is deprecated, use Meta.parse(str; kwargs...) instead.
Stacktrace:
 [1] depwarn(::String, ::Symbol) at ./deprecated.jl:68
 [2] #parse#890(::Array{Any,1}, ::Function, ::String) at ./deprecated.jl:56
 [3] parse(::String) at ./deprecated.jl:56
 [4] docsxref(::Base.Markdown.Link, ::String, ::Dict{Symbol,Any}, ::Documenter.Documents.Page, ::Documenter.Documents.Document) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/CrossReferences.jl:122
 [5] basicxref(::Base.Markdown.Link, ::Dict{Symbol,Any}, ::Documenter.Documents.Page, ::Documenter.Documents.Document) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/CrossReferences.jl:53
 [6] xref(::Base.Markdown.Link, ::Dict{Symbol,Any}, ::Documenter.Documents.Page, ::Documenter.Documents.Document) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/CrossReferences.jl:45
 [7] walk(::getfield(Documenter.CrossReferences, Symbol("##1#2")){Documenter.Documents.Page,Documenter.Documents.Document}, ::Dict{Symbol,Any}, ::Base.Markdown.Link) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Walkers.jl:66
 [8] walk(::Function, ::Dict{Symbol,Any}, ::Array{Any,1}) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Walkers.jl:36
 [9] walk(::getfield(Documenter.CrossReferences, Symbol("##1#2")){Documenter.Documents.Page,Documenter.Documents.Document}, ::Dict{Symbol,Any}, ::Base.Markdown.Paragraph) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Walkers.jl:45
 [10] crossref(::Base.Markdown.Paragraph, ::Documenter.Documents.Page, ::Documenter.Documents.Document) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/CrossReferences.jl:34
 [11] crossref(::Documenter.Documents.Document) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/CrossReferences.jl:28
 [12] @generated body at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Selectors.jl:164 [inlined]
 [13] dispatch(::Type{Documenter.Builder.DocumentPipeline}, ::Documenter.Documents.Document) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Selectors.jl:164
 [14] cd(::getfield(Documenter, Symbol("##2#3")){Documenter.Documents.Document}, ::String) at ./file.jl:70
 [15] #makedocs#1 at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Documenter.jl:186 [inlined]
 [16] (::getfield(Documenter, Symbol("#kw##makedocs")))(::Array{Any,1}, ::typeof(Documenter.makedocs)) at ./<missing>:0
 [17] top-level scope
 [18] include_relative(::Module, ::String) at ./loading.jl:510
 [19] include(::Module, ::String) at ./sysimg.jl:15
 [20] process_options(::Base.JLOptions) at ./client.jl:324
 [21] _start() at ./client.jl:375
in expression starting at /Users/amitm/Julia/julia/doc/make.jl:156
 !! No doc found for reference '[`Base.require`](@ref)'. [src/devdocs/require.md]
 !! No doc found for reference '[`RemoteChannel`](@ref)'. [src/stdlib/distributed.md]
 !! No doc found for reference '[`Future`](@ref)'. [src/stdlib/distributed.md]
 !! No doc found for reference '[`Base.require`](@ref)'. [src/manual/environment-variables.md]
 !! No doc found for reference '[`Base.require`](@ref)'. [src/manual/environment-variables.md]

WARNING: deprecated syntax "(Argtypes...)".
Use "(Argtypes...,)" instead.

WARNING: deprecated syntax "(Argtypes...)".
Use "(Argtypes...,)" instead.

WARNING: deprecated syntax "(Argtypes...)".
Use "(Argtypes...,)" instead.

WARNING: deprecated syntax "(Argtypes...)".
Use "(Argtypes...,)" instead.

WARNING: deprecated syntax "(Argtypes...)".
Use "(Argtypes...,)" instead.
Documenter: running document checks.
 !! Skipped doctesting.
 > checking footnote links.
Documenter: populating indices.
ERROR: LoadError: `makedocs` encountered errors. Terminating build
Stacktrace:
 [1] runner at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Builder.jl:202 [inlined]
 [2] @generated body at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Selectors.jl:164 [inlined]
 [3] dispatch(::Type{Documenter.Builder.DocumentPipeline}, ::Documenter.Documents.Document) at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Selectors.jl:164
 [4] cd(::getfield(Documenter, Symbol("##2#3")){Documenter.Documents.Document}, ::String) at ./file.jl:70
 [5] #makedocs#1 at /Users/amitm/Julia/julia/doc/deps/v0.7/Documenter/src/Documenter.jl:186 [inlined]
 [6] (::getfield(Documenter, Symbol("#kw##makedocs")))(::Array{Any,1}, ::typeof(Documenter.makedocs)) at ./<missing>:0
 [7] top-level scope
 [8] include_relative(::Module, ::String) at ./loading.jl:510
 [9] include(::Module, ::String) at ./sysimg.jl:15
 [10] process_options(::Base.JLOptions) at ./client.jl:324
 [11] _start() at ./client.jl:375
in expression starting at /Users/amitm/Julia/julia/doc/make.jl:156
make[1]: *** [html] Error 1
make: *** [docs] Error 2

@vchuravy
Copy link
Member

I am also seeing:

	From worker 15:	ERROR: LoadError: LoadError: invalid subtyping in definition of UnixDomainCM
	From worker 15:	Stacktrace:
	From worker 15:	 [1] top-level scope
	From worker 15:	 [2] include_relative(::Module, ::String) at ./loading.jl:510
	From worker 15:	 [3] include(::Module, ::String) at ./sysimg.jl:15
	From worker 15:	 [4] include(::String) at ./sysimg.jl:54
	From worker 15:	 [5] top-level scope
	From worker 15:	 [6] include_relative(::Module, ::String) at ./loading.jl:510
	From worker 15:	 [7] include(::Module, ::String) at ./sysimg.jl:15
	From worker 15:	 [8] process_options(::Base.JLOptions) at ./client.jl:324
	From worker 15:	 [9] _start() at ./client.jl:375
	From worker 15:	in expression starting at /usr/home/julia/julia-fbsd-buildbot/worker/11rel-amd64/build/usr/share/doc/julia/examples/clustermanager/simple/UnixDomainCM.jl:5
	From worker 15:	in expression starting at /usr/home/julia/julia-fbsd-buildbot/worker/11rel-amd64/build/usr/share/doc/julia/examples/clustermanager/simple/test_simple.jl:4

@amitmurthy
Copy link
Contributor Author

Need help fixing these errors with make docs on this PR.

 !! No doc found for reference '[`RemoteChannel`](@ref)'. [src/stdlib/distributed.md]
 !! No doc found for reference '[`Future`](@ref)'. [src/stdlib/distributed.md]

Other functions exported from Distribued are referenced fine. Are types treated differently?

@amitmurthy amitmurthy force-pushed the amitm/del_dist branch 2 times, most recently from 11057af to 1661e00 Compare November 24, 2017 07:03
@amitmurthy
Copy link
Contributor Author

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.

@amitmurthy amitmurthy requested a review from vtjnash November 24, 2017 13:02
@@ -0,0 +1,47 @@

# Multi-Threading
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

:) my mistake then.

@ViralBShah
Copy link
Member

Still some conflicts before merging.

@amitmurthy
Copy link
Contributor Author

Travis linux is green. I guess Travis OSX and AV have been broken for sometime now?

@vtjnash
Copy link
Member

vtjnash commented Nov 25, 2017

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.

@amitmurthy
Copy link
Contributor Author

@vtjnash can you also review this PR?

@amitmurthy amitmurthy force-pushed the amitm/del_dist branch 2 times, most recently from 65b5fdc to 2baebf5 Compare November 27, 2017 18:20
@ViralBShah
Copy link
Member

ViralBShah commented Nov 30, 2017

@amitmurthy Is there any specific part that needs review? Are the windows failures related? They don't appear to be.

@ViralBShah
Copy link
Member

Also needs to be rebased.

@amitmurthy
Copy link
Contributor Author

@ViralBShah , @vtjnash specifically loading.jl, client.jl and test files other than those related to distributed.

@rfourquet rfourquet mentioned this pull request Dec 1, 2017
5 tasks
@@ -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"
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

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.

@ViralBShah
Copy link
Member

Bump. Would be nice to get this excision done by the Dec 15 feature freeze.

@amitmurthy
Copy link
Contributor Author

I'll rebase and merge it once it passes CI. Can optimize post 15th.

@amitmurthy amitmurthy force-pushed the amitm/del_dist branch 2 times, most recently from a461a71 to 6bf6be4 Compare December 13, 2017 18:04
@amitmurthy
Copy link
Contributor Author

AV and Travis(Linux) are green. Travis(OSX) is the spawn test failing. Will merge in a few hours if there are no objections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
excision Removal of code from Base or the repository stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants