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.

Sign up for GitHub

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

RFC: Fixed relative path include on remote machines #21832

Merged
merged 9 commits into from
Jun 27, 2017
Merged

RFC: Fixed relative path include on remote machines #21832

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d86b37c
Fixed relative path include on remote machines
Gollor May 13, 2017
7e7393c
Removed trailing whitespace
Gollor May 13, 2017
6a4acc0
Added tests for remote including
Gollor May 13, 2017
93cf6d9
Remote include tests use temp files now
Gollor May 13, 2017
2379294
Fixed remote include tests
Gollor May 13, 2017
7ff6322
Remote include tests close io streams
Gollor May 13, 2017
49abbf3
Added @vtjnash code except for relative path in dependency tracker
Gollor May 17, 2017
eadb699
Removed explicitly absolute path
Gollor May 17, 2017
1ce1d75
Test trigger.
Gollor May 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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)))
    end

Copy link
Contributor Author

@Gollor Gollor May 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@vtjnash Looks nice. But is there a reason why the list of dependencies receives the regular path instead of explicitly absolute one?

Copy link
Member

Choose a reason for hiding this comment

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

because abspath(abspath()) seems unnecessary

path = (prev === nothing) ? abspath(_path) : joinpath(dirname(prev), _path)
if myid() == 1 && _track_dependencies[]
apath = abspath(path)
Expand Down
43 changes: 43 additions & 0 deletions test/distributed_exec.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1569,6 +1569,49 @@ catch ex
@test ex.captured.ex.exceptions[2].ex == UndefVarError(:DontExistOn1)
end

@test let
# creates a new worker in the same folder and tries to include file on both procs
working_directory = pwd()
cd(tempdir())
try
tmp_file = relpath(mktemp()[1])
proc = addprocs_with_testenv(1)
include(tmp_file)
remotecall_fetch(include, proc[1], tmp_file)
rmprocs(proc)
rm(tmp_file)
cd(working_directory)
return true
catch e
try rm(tmp_file) end
cd(working_directory)
return false
end
end == true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test that passes on both versions.


@test let
# creates a new worker in the different folder and tries to include file on both procs
working_directory = pwd()
cd(tempdir())
Copy link
Contributor

@tkelman tkelman May 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

cd(tempdir()) do
   ...
end

will automatically go back to the original working directory even on failure, bit cleaner

try
tmp_file = relpath(mktemp()[1])
tmp_dir = relpath(mktempdir())
proc = addprocs_with_testenv(1, dir=tmp_dir)
include(tmp_file)
remotecall_fetch(include, proc[1], tmp_file)
rmprocs(proc)
rm(tmp_dir)
rm(tmp_file)
cd(working_directory)
return true
catch e
try rm(tmp_dir) end
Copy link
Contributor

Choose a reason for hiding this comment

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

better as rm(tmp_dir, force=true) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that should be better.

try rm(tmp_file) end
cd(working_directory)
return false
end
end == true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test that requires the fix to pass.


# Run topology tests last after removing all workers, since a given
# cluster at any time only supports a single topology.
rmprocs(workers())
Expand Down