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

Load module on remote workers from master #21647

Closed
wants to merge 68 commits into from
Closed

Load module on remote workers from master #21647

wants to merge 68 commits into from

Conversation

GregPlowman
Copy link
Contributor

Fix #19960

@ararslan ararslan added the parallelism Parallel or distributed computation label Apr 30, 2017
@tkelman tkelman added the needs tests Unit tests are required for this change label Apr 30, 2017
@amitmurthy
Copy link
Contributor

@vtjnash is the right person to review this.

@GregPlowman
Copy link
Contributor Author

Testing is quite tricky as it requires workers on remote hosts.

I have something that might work (see below), but I don't know much about the test environment.
I've looked at test/distributed_exec.jl but I can't see where there are remote workers.
The section for ssh simulates different hosts with ["localhost", string(getipaddr()), "127.0.0.1"], but these hosts are all the local machine aren't they?

addprocs(hosts ...) # add workers on remote hosts

begin
    load_dir = tempdir()
    push!(LOAD_PATH, load_dir)

    function create_module(module_name, module_body, precompile::Bool)
        module_file = joinpath(load_dir, module_name, "src", string(module_name, ".jl"))
        isdir(dirname(module_file)) || mkpath(dirname(module_file))
        module_code = """
            __precompile__($precompile)
            module $module_name
                $module_body
            end
        """
        write(module_file, module_code)
    end

    create_module("Issue19960A", "export f; f() = myid()", true)
    create_module("Issue19960B", "export g; using Issue19960A; g() = f()", true)

    using Issue19960B

    @test g() == 1
    for wid in workers()
        @test remotecall_fetch(g, wid) == wid
    end
end

@GregPlowman
Copy link
Contributor Author

Is it possible this could be backported to v0.5 (v0.5.2)?

@GregPlowman GregPlowman closed this May 2, 2017
@GregPlowman
Copy link
Contributor Author

Rerun tests

Test for Issue #19960
end

catch
@test false
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the catch here at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given your comment, it probably doesn't need to be there.
(As you've probably guessed I don't really understand testing)
I thought that there had to be an explicit @test failure, rather than just a thrown error from the try block.
I can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

something went wrong with the rebase as well, when you do git rebase -i origin/master you should remove all the commits that aren't yours from the list that shows up, and when you're happy with the local state of the branch (just your commits, on top of latest master) you can push to the glp/remote_loading branch of your fork with --force

Copy link
Contributor Author

@GregPlowman GregPlowman May 4, 2017

Choose a reason for hiding this comment

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

Yes I can see that it's screwed up.
But also test/compile.jl is showing extra changes (changes made to master after my original PR but before the update). I don't think those changes should be showing here.
I don't know how to get out of this mess, except closing this and making a new PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can amend a pull request in place with an interactive rebase, usually git rebase -i origin/master assuming origin is JuliaLang/julia and you've done a git fetch origin recently. Once the local copy is cleaned up, do git push --force GregPlowman branchname assuming GregPlowman is a remote for your fork

Copy link
Contributor Author

@GregPlowman GregPlowman May 8, 2017

Choose a reason for hiding this comment

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

Thanks Tony!
I've already created a replacement PR, but I'll certainly try to follow your advice next time.

@GregPlowman
Copy link
Contributor Author

Replaced by #21695

@GregPlowman GregPlowman closed this May 4, 2017
@tkelman tkelman removed backport pending 0.5 needs tests Unit tests are required for this change labels May 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parallelism Parallel or distributed computation
Projects
None yet
Development

Successfully merging this pull request may close these issues.