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

WIP: Include support for cross-platform deps.jl #130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rened
Copy link
Contributor

@rened rened commented Mar 14, 2015

Given two machines Mac and Linux with the respective OSes, the following currently does not work:

  • addproc({"Linux"}) on the Mac
  • using HDF5

The problem is that the auto-generated deps.jl file, which is included by HDF5/src/plain.jl on PID 1, has the path to the dynamic library hard-coded, and every worker sees exactly that string, not the one which corresponds to the worker's machine.

(HDF5 is just used as an example here, and any OS/package-manager mismatch where workers have their libraries at different paths would fail as well)

This PR includes 2 changes:

  • if the generated deps.jl cannot load the hard-coded path, it looks for the local installation of the package and uses its deps.jl
  • a helper function include_deps(pkgname) which can be used by HDF5 to load its library (see update for cross platform bindeps JuliaIO/HDF5.jl#221), which make sure to use the local deps.jl

Only drawback I found so far is that existing build.jl scripts need to be updated to include the package name when calling @BinDeps.install - any ideas how to get around this?

@mlubin
Copy link
Contributor

mlubin commented Mar 14, 2015

@rened, could you restore the old behavior if the package name isn't provided? No need to break things.

@mlubin
Copy link
Contributor

mlubin commented Mar 14, 2015

Also the include_deps pattern defeats the point of deps.jl which was to avoid needing to load the BinDeps package.

@rened
Copy link
Contributor Author

rened commented Mar 15, 2015

Sure, I'll include the default behavior for install.
I was not aware that BinDeps should not be needed at runtime, I'll try to refactor that so that it still works.

@tkelman
Copy link
Contributor

tkelman commented Mar 15, 2015

This should also avoid using Pkg.dir, and rather make use of @__FILE__ for packages that might get installed in non-default locations, see JuliaLang/julia#8679 (comment)

I really don't think mixed-platform addprocs is something that base Julia claims to support, and adding complication to the already-difficult-to-maintain bindeps system to accommodate this use case seems iffy to me.

@rened
Copy link
Contributor Author

rened commented Mar 15, 2015

I'll try to make is as uninvasive as possible. I believe having the ability to work on your own machine (with your favorite OS / distribution), and then quickly summon some machines from a cluster (oder simply other random machines in the lab) would be really neat to have. Let's see whether there is a simple and robust way to do it.

@rened rened changed the title Include support for cross-platform deps.jl WIP: Include support for cross-platform deps.jl Mar 15, 2015
@samoconnor
Copy link

I believe having the ability to work on your own machine (with your favorite OS / distribution), and then quickly summon some machines from a cluster (oder simply other random machines in the lab) would be really neat to have.

I think that's a pretty essential requirement.
Linux is clearly the dominant OS for server/cloud/cluster deployment, but it is not what is installed on most laptops. Even if an end-user machine is running Linux, it is likely to be a different distribution/version to the one used by the cluster/cloud provider.

function include_deps(pkg::String)
depsfile = joinpath(Pkg.dir(pkg),"deps/deps.jl")
if isfile(depsfile)
include_string(readall(depsfile))
Copy link
Member

Choose a reason for hiding this comment

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

Why not just include(depsfile)?

@nalimilan
Copy link
Member

While I'm sympathetic to the goal, this doesn't sound like the right level to support this. It doesn't make much sense to have @checked_lib take a path as an argument, but load it from deps.jl in case of failure. If it's so smart, why not load deps.jl unconditionnally?

Looks like we need to rethink the system more seriously. I'm not familiar with the parallel code, but in the present case it seems that the library path (or equivalently the contents of deps.jl) shouldn't be considered as something to share between workers. The right fix is to find a way to tell Julia that it needs to load this information separately for each worker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants