RFC: Fixed relative path include on remote machines #21832
Conversation
test/distributed_exec.jl
Outdated
| try rm("temp.jl") end | ||
| return false | ||
| end | ||
| end == true |
There was a problem hiding this comment.
The test that passes on both versions.
test/distributed_exec.jl
Outdated
| try rm("temp.jl") end | ||
| return false | ||
| end | ||
| end == true |
There was a problem hiding this comment.
The test that requires the fix to pass.
test/distributed_exec.jl
Outdated
| @test let | ||
| # creates a new worker in the same folder and tries to include file on both procs | ||
| try | ||
| touch("temp.jl") |
There was a problem hiding this comment.
we should avoid assuming the current working directory is writable, in some installations it won't be - a mktemp() do block might work well here
test/distributed_exec.jl
Outdated
| cd(working_directory) | ||
| return true | ||
| catch e | ||
| try rm(tmp_dir) end |
There was a problem hiding this comment.
better as rm(tmp_dir, force=true) ?
There was a problem hiding this comment.
Right, that should be better.
test/distributed_exec.jl
Outdated
| @test let | ||
| # creates a new worker in the different folder and tries to include file on both procs | ||
| working_directory = pwd() | ||
| cd(tempdir()) |
There was a problem hiding this comment.
cd(tempdir()) do
...
end
will automatically go back to the original working directory even on failure, bit cleaner
|
I removed the |
base/loading.jl
Outdated
| @@ -263,6 +263,9 @@ const _require_dependencies = Any[] # a list of (path, mtime) tuples that are th | |||
| const _track_dependencies = Ref(false) # set this to true to track the list of file dependencies | |||
| function _include_dependency(_path::AbstractString) | |||
| prev = source_path(nothing) | |||
| if myid() != 1 && prev === nothing | |||
| prev = remotecall_fetch(abspath, 1, ".") | |||
| end | |||
There was a problem hiding this comment.
it seems like this may confuse the SOURCE_PATH pop in include_from_node1.
I think it'll be clearer to be explicit here:
prev = source_path(nothing)
if prev === nothing
if myid() == 1
path = abspath(_path)
else
path = joinpath(remotecall_fetch(abspath, 1, "."), _path)
end
else
path = joinpath(dirname(prev), _path)
end
if myid() == 1 && _track_dependencies[]
push!(_require_dependencies, (path, mtime(path)))
endThere was a problem hiding this comment.
@vtjnash Looks nice. But is there a reason why the list of dependencies receives the regular path instead of explicitly absolute one?
There was a problem hiding this comment.
because abspath(abspath()) seems unnecessary
|
@vtjnash Is this good to go? |
| end == true | ||
|
|
||
| @test let | ||
| # creates a new worker in the different folder and tries to include file |
vtjnash
added
backport pending 0.6
compiler:precompilation
|
this should have been squashed |
|
Can we PLEASE pay more attention to squashing versus not squashing when merging PRs? This is not hard to do – take a few seconds when you're merging something. |
Ref #21832 Squashed for backporting (cherry picked from commit d86b37c) (cherry picked from commit 7e7393c) (cherry picked from commit 6a4acc0) (cherry picked from commit 93cf6d9) (cherry picked from commit 2379294) (cherry picked from commit 7ff6322) (cherry picked from commit 49abbf3) (cherry picked from commit eadb699) (cherry picked from commit 1ce1d75)
Ref #21832 Squashed for backporting (cherry picked from commit d86b37c) (cherry picked from commit 7e7393c) (cherry picked from commit 6a4acc0) (cherry picked from commit 93cf6d9) (cherry picked from commit 2379294) (cherry picked from commit 7ff6322) (cherry picked from commit 49abbf3) (cherry picked from commit eadb699) (cherry picked from commit 1ce1d75)
Ref #21832 Squashed for backporting (cherry picked from commit d86b37c) (cherry picked from commit 7e7393c) (cherry picked from commit 6a4acc0) (cherry picked from commit 93cf6d9) (cherry picked from commit 2379294) (cherry picked from commit 7ff6322) (cherry picked from commit 49abbf3) (cherry picked from commit eadb699) (cherry picked from commit 1ce1d75)
The fix of #21679
This code works when the remote machine tries to include the file using the current working directory. Previously the machine was resolving the path locally, what could lead to errors with relative paths. In the case of
include("file.jl")machine remote@remotehost could try to download/home/remote/file.jlfrom the main machine local@localhost when the true file location is/home/local/file.jl.Now if the remote machine has to use working directory to include files it will use the main machine working directory instead.