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

Julia v1.0 support: Add option to specify julia version in REQUIRE #393

Merged
merged 8 commits into from Oct 1, 2018

Conversation

Projects
None yet
6 participants
@NHDaly
Copy link
Contributor

NHDaly commented Sep 7, 2018

This PR adds the option to specify a julia version in the REQUIRE file, and updates the JuliaBuildPack to support julia v0.7 and v1.0.

It also bumps the default julia version from v0.6.0 to v0.6.4 (the latest release for v0.6).

Adds tests for specifying a custom version, both for a julia 0.6.3 and julia 1.

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

Fixes #392.

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

@KristofferC could you maybe take a look at this to verify that i've handled the REQUIRE file acceptably in julia 0.7/1.0? I think it works, but it seems like there might've been a more elegant way for me to do this?

Thanks so much!! :)

@KristofferC

This comment has been minimized.

Copy link

KristofferC commented Sep 7, 2018

Would be a lot simpler if you have a Project + Manifest, which in case the whole thing would just be a Pkg.instantiate() to download all the packages at the versions specified in the Manifest.

Otherwise doing something like a Pkg.add("path/to/repo") on the path of the package itself should install all the required dependencies even if it has a REQUIRE file.

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

Would be a lot simpler if you have a Project + Manifest

I was under the impression that we're really not supposed to be using these yet, until the Great Migration moves everything from REQUIRE to Project+Manifest. Am I getting that wrong? Is it that you're supposed to use Project+Manifest in your local projects (or notebook repo in this case), but still use REQUIRE files in packages that you submit to METADATA?

Thanks, and sorry for my confusion!

In that case, how do you recommend proceeding for this code? Should we simply say we don't support REQUIRE files in your notebook repo if you're using julia v0.7+?

Otherwise doing something like a Pkg.add("path/to/repo") on the path of the package itself should install all the required dependencies even if it has a REQUIRE file.

Sorry, I don't think I understand what you mean. If it helps explain better, the repo being built in this case usually isn't a Package. Rather, it's a Project that contains .ipynb files, and the REQUIRE file is used to specify dependencies needed to run those notebooks. Here's an example repo:
https://github.com/binder-examples/julia-python

So I don't think Pkg.add is what we want, since there's no package there at all. Something like Pkg.resolve("path/to/repo") would be excellent though! :)

@betatim

This comment has been minimized.

Copy link
Collaborator

betatim commented Sep 7, 2018

Nice work and thanks for helping us get better Julia support.

For Python and R we use runtime.txt to let users choose versions. So from a uniformity point of view it would be good to use that for Julia as well. However from one of your comments it sounds like the Julia community uses the REQUIRE file for this? What do you think is the best option?

@KristofferC

This comment has been minimized.

Copy link

KristofferC commented Sep 7, 2018

I was under the impression that we're really not supposed to be using these yet, until the Great Migration moves everything from REQUIRE to Project+Manifest. Am I getting that wrong? Is it that you're supposed to use Project+Manifest in your local projects (or notebook repo in this case), but still use REQUIRE files in packages that you submit to METADATA?

You can use Project + Manifest now, it is just that the way things are registered in METADATA right now is using REQUIRE files. For notebooks, I am not sure what the point of a REQUIRE file would be on 1.0. There's no Pkg.resolve(".../REQUIRE") in Pkg3.

If it helps explain better, the repo being built in this case usually isn't a Package.

Yeah, I haven't really checked what this repo is doing heh.

Slightly related: JuliaLang/IJulia.jl#673.

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

Slightly related: JuliaLang/IJulia.jl#673.

Oh cool, thanks, yes i think that's a related conversation. Thanks for the pointer!

Yeah, I haven't really checked what this repo is doing heh.

hehe. :) Np. This repo is used by MyBinder (and probably other parts of the jupyter universe?) to instantiate a docker container for a given directory (repo) that contains .ipynb files. They currently allow you to specify dependencies that should be loaded while building the docker image -- so that you don't need to re-install them every time you run a notebook. This is done via a top-level REQUIRE for julia, an environment.yml for python, etc.

After reading the conversation you linked, I think this process is used to solve some of the issues discussed in that thread!

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

You can use Project + Manifest now, it is just that the way things are registered in METADATA right now is using REQUIRE files. For notebooks, I am not sure what the point of a REQUIRE file would be on 1.0. There's no Pkg.resolve(".../REQUIRE") in Pkg3.

Gotcha. Okay! It seems like a reasonable answer here is just to say: Sorry, if you're using v0.7+ you can't use REQUIRE files and instead have to use Project+Manifest! that seems reasonable to me. Thanks! :)

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

But also, the other thing I was hoping to change in this PR is allowing you to specify the julia version in the REQUIRE file. Currently, the way this tool works is it only allows you to pull down julia 0.6.0, and so I think allowing the user to specify which version of julia to install would be preferable.

So I think maybe the logic for this PR should instead be:

  • If there's a REQUIRE, assume the julia version to be 0.6.4. If it contains a julia version string, read the version out of it. If the listed version is >0.7, error out or something?
    • Then, copy the REQUIRE to ~/.julia/version/and do Pkg.resolve()
  • If there's a Project.toml, assume the julia version to be 1.0.0. If it contains a [compat] section with a julia version string, read the version out of it. If < 0.7, error out or something.
    • Then, copy Project.toml and possible Manifest.toml to ~/.julia/environments/version/ and do Pkg.instantiate().

Does that sound reasonable? Thanks for your help!

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

Although (sorry for the post-spam), I do think it would be reasonable to also support REQUIRE files for 0.7, at least for now, since they're still a part of the julia package ecosystem (for METADATA). So it seems reasonable to me to support both for now?

@KristofferC

This comment has been minimized.

Copy link

KristofferC commented Sep 7, 2018

The "correct" thing would be to record the julia version in the Manifest but we don't currently do that. So next best thing:

Note that julia = "1.0" defines a range of intervals in Pkg3 ([1.0 - 2.0)). So I think the way to do it is if there is no [compat] for julia use the latest, otherwise use the latest version such that it is within the range (which can be tested as):

julia> v"1.0" in Pkg.Types.semver_spec("1.0")
true

julia> v"1.3" in Pkg.Types.semver_spec("1.0")
true

julia> v"2.0" in Pkg.Types.semver_spec("1.0")
false

So if the julia versions [1.0, 1.2, 1.3, 2.0] existed and julia = 1.0 was in [compat] then use 1.3.

With this, the julia version used will change as new versions get released (but that is not any different from what Pkg.resolve did on 0.6 when new package version was released though).

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

Oh, cool, yeah that makes sense. That should be an easy change. So how does this behavior sound:

  • If there's a Project.toml:
    • If it contains a [compat] section with a julia version string, read the version out of it. If < 0.7, error out or print or warning or something.
      • Install the latest version of julia that satisfies the provided semver specification. (e.g if julia versions [1.0, 1.2, 1.3, 2.0] existed, and julia = 1.0 in [compat], install 1.3.)
    • Otherwise, just install the latest version of julia.
    • Then, copy the Project.toml and possible Manifest.toml to ~/.julia/environments/version/, open julia, and run Pkg.instantiate().
  • If there's a REQUIRE file (and no Project.toml):
    • If it contains a julia version string, install the latest version that matches the prefix. (e.g. julia 0.6 installs v0.6.4, julia 0.7 installs v0.7.0 and julia 0.6.3 installs v0.6.3.)
    • Otherwise, install the latest 0.6 (v0.6.4).
    • Then, if version < 0.7, copy the REQUIRE to ~/.julia/version/ and do Pkg.resolve()
    • Else (version >0.7), call Pkg.add() on each line of the REQUIRE, using the code I've currently implemented in this PR, here.

I think this sounds like a good approach that is the most permissive of user behavior. But it's seems reasonable to say it's too permissive, and that we should disallow using REQUIRE if you're going to use julia 0.7. I'm open to your suggestions there! :)

Thanks again for your help!

@KristofferC

This comment has been minimized.

Copy link

KristofferC commented Sep 7, 2018

Then, copy the Project.toml and possible Manifest.toml to ~/.julia/environments/version/, open julia, and run Pkg.instantiate().

There is no need to copy. You can set the JULIA_PROJECT env variable to the project path, start julia with julia --project=/path/to/project or after starting julia runPkg.activate("path/to/project") and then that will be the active project so instantiating will look at that path then. If the Project is in the pwd() then julia --project is enough (implies pwd).

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

There is no need to copy. You can set the JULIA_PROJECT env variable to the project path, start julia with julia --project=/path/to/project or after starting julia runPkg.activate("path/to/project") and then that will be the active project. If the Project is in the pwd() then julia --project is enough (implies pwd).

Hmm, yeah I know about that, but I am wondering if that will work correctly with the jupyter notebook kernels... Maybe it will? Basically, the goal is that whenever the user opens a .ipynb and starts the Julia kernel, that it gets an environment with all the right packages installed.

If I start julia with the JULIA_PROJECT env variable set when I first install IJulia, will it set things up so that the kernel it installs will always point to the right environment? Am I understanding that correctly?

@KristofferC

This comment has been minimized.

Copy link

KristofferC commented Sep 7, 2018

I don't know what configuration options you have over the jupyter kernel file for julia that IJulia generates from here, but if you can do zero configuration then you would indeed have to put the Project + Manifest in the default place (environments/...).

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 7, 2018

Okay cool, that makes sense. thanks.

Alright, well, let's leave aside this issue, since it's relatively minor. Would you mind giving me your thoughts on the overall pseudocode behavior above? I appreciate your help!! :) :)

@KristofferC

This comment has been minimized.

Copy link

KristofferC commented Sep 7, 2018

I would just not bother with 0.6 but if you really want to then that pseudo code seems ok. REQUIRE files however have no implicit upper bounds like the Pkg3 compat has, so julia 0.6 means julia [0.6, 99999999...) so it is a bit tricky to know what Julia version to install. But as a heuristic, the latest 0.6 version is probably the right thing to do.

@yuvipanda

This comment has been minimized.

Copy link
Member

yuvipanda commented Sep 9, 2018

@betatim we should probably use whatever is canonical in the language for specifying version, falling back to runtime.txt only when there is no canonical way. environment.yml for example has a canonical way.

@NHDaly @KristofferC <3 for working on this! Have you run repo2docker locally to verify that IJulia works as intended?

for pkg in keys(Pkg.Reqs.parse("%(require)s")) \
pkg != "julia" && eval(:(using $(Symbol(pkg)))) \
end \
require_file = "%(require)s" ;\

This comment has been minimized.

@yuvipanda

yuvipanda Sep 9, 2018

Member

I think this is now complex enough to be moved to a file of its own and then called from here. This lets us stop mixing Julia and Bash. See https://github.com/jupyter/repo2docker/blob/master/repo2docker/buildpacks/conda/__init__.py#L83 for how to add a script to the repo. Then this script (which should be in Julia) can be called from here.

This comment has been minimized.

@NHDaly

NHDaly Sep 12, 2018

Contributor

Awesome, great idea. I'm pushing up a commit now that just makes this change: splitting it out into its own file! :) I moved them into a new folder, buildpacks/julia.

Good suggestion! :)


It seems to be working correctly, except that the line I added to rm /tmp/install-repo-dependencies.jl is failing:

rm: cannot remove '/tmp/install-repo-dependencies.jl': Operation not permitted
The command '/bin/sh -c julia /tmp/install-repo-dependencies.jl "REQUIRE" && rm /tmp/install-repo-dependencies.jl' returned a non-zero code: 1F

So now the tests are failing.

Any ideas? It seems like this file has the same permissions as conda/environment.frozen.yml, which is deleted successfully by the conda buildpack here.

I see that the python buildpack copies its files to /home/main.. Should I do that instead?

This comment has been minimized.

This comment has been minimized.

@NHDaly

NHDaly Sep 12, 2018

Contributor

@yuvipanda Also, sorry, i should've tagged you to make sure you can see this question. :)

Do you have any advice on what i should do here?

NHDaly added some commits Sep 12, 2018

Split out julia deps installation into own file.
Moved julia BuildPack script into its own directory,
`repo2docker/buildpacks/julia/`.

Split out the logic for finding a repo's required Julia dependencies and
installing them into a separate file: install-repo-dependencies.jl
@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 12, 2018

@betatim we should probably use whatever is canonical in the language for specifying version, falling back to runtime.txt only when there is no canonical way. environment.yml for example has a canonical way.

Oh yeah, I forgot about runtime.txt... So that should be added as a fallback mechanism? Right now, without this PR, is that already a thing? Could I have just specified the Julia version via runtime.txt?

@NHDaly @KristofferC <3 for working on this! Have you run repo2docker locally to verify that IJulia works as intended?

haha <3 thanks!! :) I'll be happy to get it updated!

Yes, I have run it locally to verify that the tests (including those I added) all pass, and I launched a jupyter notebook in one of the images it created, and it worked correctly with different julia versions!

(That said, the contributing guide doesn't explicitly say how to actually launch a notebook after creating the docker image, it only covers tests. I just did a docker container ls to find one of the ones created by the tests, and then launched that one.)


I'm going to put in some work in the next couple days to update the logic to what Kristoffer and I discussed above. Do I also need to add in logic to handle falling back to runtime.txt, or does that behavior come automatically for free? Thanks!

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 12, 2018

@KristofferC: One more question, should we require people use JuliaProject.toml here, since the whole point of this file is to tell repo2docker "Hey this repo needs Julia installed! Plz install this version and these deps."? Or should we support either Project.toml or JuliaProject.toml?

What other languages use Project.toml, and is there an easy way to read the file and tell if it's intended for julia or some other language?

@yuvipanda

This comment has been minimized.

Copy link
Member

yuvipanda commented Sep 12, 2018

@NHDaly awesome. I would recommend against adding any runtime.txt logic right now. Let us know when this is ready for review again!

@KristofferC

This comment has been minimized.

Copy link

KristofferC commented Sep 12, 2018

Pkg and code loading in Julia Base support both names. FWIW, I personally argued that only using JuliaProject.toml was the way to go but oh well. For now, I think it is easiest to use JuliaProject.toml if it exists and otherwise use Project.toml

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 12, 2018

Haha yeah, i mean, I can see why you argued for that. :/ oh well indeed.

Okay great, thanks for the guidance, that makes sense to me as well. :)

"""
)
]
require = "/Users/daly/Documents/developer/website/nhdaly.github.io/notebooks/REQUIRE"

This comment has been minimized.

@minrk

minrk Sep 17, 2018

Member

leftover debug lines?

This comment has been minimized.

@NHDaly

NHDaly Sep 27, 2018

Contributor

Eeep, thanks @minrk! Deleted.

```

Each one will be installed but **not** pre-compiled. If you'd like to
pre-compile your Julia packages, consider using a ``postBuild`` file.

This comment has been minimized.

@betatim

This comment has been minimized.

@NHDaly

NHDaly Sep 27, 2018

Contributor

Huh! yeah good point. Thanks. I'll delete that!

It looks like all packages are always precompiled, and have been since this PR:
72466e1#diff-645542c2db4cb5cddff30d5f209c2514R613

I preserved that behavior in this PR. So yeah, i'll delete that line, thanks!

This comment has been minimized.

@NHDaly

NHDaly Sep 27, 2018

Contributor

Fixed.

@betatim

This comment has been minimized.

Copy link
Collaborator

betatim commented Sep 27, 2018

All tests pass, some small nits, merge after those?

What is the new default Julia version going to be when the user does not specify anything? Should we bump it to 1.0 or leave it at the (now quite old) 0.6 that has been the default?

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 27, 2018

All tests pass, some small nits, merge after those?

Ah, actually, this still needs some pretty major changes: adding support for the Project.toml files per this comment:
#393 (comment)

I started working on it again yesterday, but it's actually been a bit tough. The hardest part was trying to handle the "semver" logic in python that the .toml files use for specifying a julia version. (Needs to be done in python, since it's deciding which version of julia to install!)

But actually, I think maybe you're right that we could just merge this PR as-is, and I could fork the changes I've been working on into a second PR. That might actually make more sense, just to allow getting something working sooner rather than later! What do you think? Should I split that into a separate PR?

What is the new default Julia version going to be when the user does not specify anything? Should we bump it to 1.0 or leave it at the (now quite old) 0.6 that has been the default?

I had kept it at 0.6 so as not to break anyone currently using the system. I think it is perfectly reasonable (and probably preferable) to bump it, but I assume we'd want to publish that somewhere first? Like, HEY default julia versions are going to bump in a week, please use a REQUIRE file to keep yourself at 0.6 if that's what you really want?

As for which version, no I think we should go to 0.7, since a lot of packages still aren't yet 1.0 compatible. I think we could probably bump to 1.0 in a month or two.

@betatim

This comment has been minimized.

Copy link
Collaborator

betatim commented Sep 27, 2018

As for which version, no I think we should go to 0.7, since a lot of packages still aren't yet 1.0 compatible. I think we could probably bump to 1.0 in a month or two.

Ok, maybe we stick with the current default and use our "one chance" to annoy people by bumping straight to 1.0.

What do you think? Should I split that into a separate PR?

So now we can choose the version via the first line in REQUIRE and the second PR would add support for {Julia}Project.toml? If the answer is yes I'd vote to merge now and make a second PR. (Smaller PRs are the better PRs 😄)

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 27, 2018

Smaller PRs are the better PRs 😄

Haha yes agreed, 100%. :)

So now we can choose the version via the first line in REQUIRE and the second PR would add support for {Julia}Project.toml? If the answer is yes I'd vote to merge now and make a second PR.

Yes the answer is yes! :)

Okay, then, before you merge, allow me the chance to address these remaining nits. I'll have something pushed up in a few hours! :)

Ok, maybe we stick with the current default and use our "one chance" to annoy people by bumping straight to 1.0.

Haha okay yeah I think that could make sense.. The one interesting and weird thing to note about this version change is that 0.7 is closer to 1.0 than to 0.6. 0.7 is basically "1.0 with deprecations", or a backwards-compatible 1.0.

So given that, I think I agree that the 1.0 bump definitely should be announced, but actually that we could probably silently bump the default to 0.7 and people would on the whole be glad, not sad. There are fewer and fewer existing use cases where people would prefer 0.6 over 0.7.

So maybe we submit this, send a silent PR to bump to 0.7, and then eventually do an announced PR to bump to 1.0?

Remove accidental testing lines & HACK to fix `rm`
Fixes the tests w/ hack: disabled `rm` installation file. Needs to
be fixed before submitting

Adds TODO for Project.toml support
@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Sep 27, 2018

One more thing to do: I realized while playing with Project.toml that some of the environment variables needed by julia have changed between 0.6 and 0.7/1.0.

Do you think it's safe to just always define all of them as a simple solution? (for example, JULIA_HOME is now renamed to JULIA_BINDIR, so can we just define both?)

@betatim

This comment has been minimized.

Copy link
Collaborator

betatim commented Sep 27, 2018

Do you think it's safe to just always define all of them as a simple solution? (for example, JULIA_HOME is now renamed to JULIA_BINDIR, so can we just define both?)

From a repo2docker point of view: sure. Though it sounds like it would be more of a question for Julia people? Maybe add specific comments to the ones that we can remove at some point when we remove 0.6 support? As a form of forward looking house keeping.

@NHDaly NHDaly force-pushed the NHDaly:julia_version_customization branch from eefaa6d to a918b53 Sep 27, 2018

@NHDaly NHDaly force-pushed the NHDaly:julia_version_customization branch from a918b53 to 55b8423 Sep 27, 2018

Add JULIA_BINDDIR environment variable for julia v0.7+
Add tests for the ENVIRONMENT VARIABLES to the julia unit tests.

@NHDaly NHDaly force-pushed the NHDaly:julia_version_customization branch from b5d8d5a to 7fbb0f9 Oct 1, 2018

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Oct 1, 2018

Maybe add specific comments to the ones that we can remove at some point when we remove 0.6 support? As a form of forward looking house keeping.

Done!

Alright, I think this is all set to be merged! I'm sorry for the delay on just that last piece.

This PR adds unit tests for specifying a specific old julia version (v0.6.3) and for specifying using the new julia version, v1.0.0, as well as testing that the environment variables are set correctly for both julia <=0.6 and >=0.7.

I have also verified manually that the julia notebooks run correctly from the docker image created by each of the unit tests! :)

So merge away! :)

@betatim betatim merged commit 32e5ef2 into jupyter:master Oct 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@betatim

This comment has been minimized.

Copy link
Collaborator

betatim commented Oct 1, 2018

Merged!

Do you want to deploy these changes to mybinder.org directly? If yes we have instructions for deploying that should explain how to deploy the new revision of repo2docker.

@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented Oct 1, 2018

wahooooo! Way to go!

@NHDaly NHDaly deleted the NHDaly:julia_version_customization branch Oct 1, 2018

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Oct 1, 2018

Hooray! Thanks Tim! :)

Do you want to deploy these changes to mybinder.org directly?

Yes, I think so! Is there anything I should consider as to why I might not want to do that? Thanks!

@betatim

This comment has been minimized.

Copy link
Collaborator

betatim commented Oct 1, 2018

Yes, I think so! Is there anything I should consider as to why I might not want to do that? Thanks!

I don't think so. We can always revert it when we notice a bug (or fix the bug and deploy the fix). 🚨beware: the feeling of responsibility and excitement from deploying a change you made to production where lots of people will use it straight away can be addictive 😄.

I'd suggest you drop by https://gitter.im/jupyterhub/mybinder.org-deploy and give people a short heads up about the deploy when you are ready. We use the chat as a kind of log book and source of "what has been going on". We try and group deploys to Tuesdays and Thursdays, mostly so that people can plan a bit to be available in case follow up is needed (luckily tomorrow is Tuesday!)

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Oct 3, 2018

Thanks for the explanation!

beware: the feeling of responsibility and excitement from deploying a change you made to production where lots of people will use it straight away can be addictive 😄

Haha 😋 I will beware, indeed!

Ah, sorry i missed today! Actually, if you don't mind, would you be able to deploy this? Or just allow it to be deployed the next time you have something to deploy? I'm a bit busy this week. :) No problem if you can't; i'll look at it this weekend!

@betatim

This comment has been minimized.

Copy link
Collaborator

betatim commented Oct 3, 2018

This has been deployed: jupyterhub/mybinder.org-deploy#758

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Oct 3, 2018

Thanks a lot Tim!! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment