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

[MRG+1] Split julia support into pre julia 1.0 and post julia 1.0 #595

Merged
merged 52 commits into from Mar 6, 2019

Conversation

@davidanthoff
Copy link
Contributor

davidanthoff commented Feb 26, 2019

This is an alternative to #594, that I think is cleaner.

@betatim This doesn't follow your suggestion of composition, instead it just treats these two julia versions as entirely different stories. I think we probably eventually want to just remove support for version pre 1.0: they are not maintained, and I have a hard time imagining that anyone will use them for much longer. Removing support for this old legacy stuff would be super easy with the code structure in this PR: one wouldn't even have to touch the julia 1.0 support code at all. But we can also leave it in indefinitely, given that support for the two is more or less completely isolated, it probably doesn't do any harm.

The logic of which of the julia implementations should handle a given repo is super simple, and I don't think there is a risk that they would get in each others way.

There is also a more radical suggestion: we could say that a REQUIRE file can only be used for julia 0.6.x, and for julia 1.0 one must use a Project.toml. If we were designing this from scratch, I think that would actually be the proper way to handle things. REQUIRE is really not a thing in julia 1.0 anymore (except for some corner package cases that are not relevant to the binder story), so the REQUIRE support for julia 1.0 here in binder is really a binder-only thing, which doesn't seem ideal. But on the other hand, support for this has been in the shipping version of binder for a while now, so maybe we just have to keep it for backwards compat reasons?

The major thing that is still missing from this PR is the logic that selects the exact julia 1.x.x version subject to the version specifier in the Project.toml. Things that we need for that are a) add a dependency to https://github.com/uiri/toml so that I can read the toml file, and b) figure out whether there is a python semver package that can help us do the version resolution.

I'm stuck right now on a) already, because I don't know what I need to do to add a package dependency :) I'm not really a Python expert ;)

The support for julia 1.0 here has some other improvements: 1) I managed to install the Julia kernel into the right location right away, no manual file move needed anymore, 2) we don't need the julia code that resolves the repo dependencies, the package manager handles all of that now.

Random other question: why does the julia build pack inherit from the python build pack? Wouldn't it make more sense to inherit from something that doesn't install the python kernel? I.e. something that does install the jupyter notebook & lab, but not the python kernel itself?

Also CC @NHDaly.

@NHDaly

This comment has been minimized.

Copy link
Contributor

NHDaly commented Feb 26, 2019

@davidanthoff: Thanks so much for putting work into this!!

First off, I'm sorry i haven't been a part of this conversation; I'm on vacation this week and last, and haven't been looking at a computer much. This looks really great though, thanks for your work!

Second apology: I'm sorry I never posted a PR for it, but I actually started some work on this a while ago but never posted it! You can see my take on it here:
master...NHDaly:julia_projecttoml

The major thing that is still missing from this PR is the logic that selects the exact julia 1.x.x version subject to the version specifier in the Project.toml. Things that we need for that are a) add a dependency to https://github.com/uiri/toml so that I can read the toml file, and b) figure out whether there is a python semver package that can help us do the version resolution.

I'm stuck right now on a) already, because I don't know what I need to do to add a package dependency :) I'm not really a Python expert ;)

In particular I wrote a python library for (b), because I couldn't find a python semver library either that worked how we want it to for Julia. But i'm also not a super python expert and actually getting someone to check this over is one of the main reasons I never opened a PR with these changes. :)

Sorry i don't have time to look at this PR for a couple days (still on vacation), but i will! In the meantime, hopefully looking at my work will help some! I THINK it's a reasonable python implementation of Julia's semver.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Feb 26, 2019

I think keeping support "for longer than is reasonable" should be the strategy for repo2docker given we are in the business of reproducible science. Once we have had support for repositories specifying the version of repo2docker they want to use to build we can get more aggressive/less conservative.

I wouldn't add the julialegacy sub-directory and instead create two files in julia/ one each per build pack (and a __init__.py). I'd also avoid using "legacy" because what happens when there is v1.2, does 1.0 now belong to legacy or not? Maybe JuliaProjectTomlBuildPack and JuliaREQUIREBuildPack instead using names with the version in it? It seems like the build packs are selected by different files and not versions.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Feb 26, 2019

After reading this one more time and pondering it I think we should name the build packs after the files that trigger them instead of versions because you can install Julia 1.0 and 1.1 via REQUIRE as well as a Project.toml.

I think adding toml as a dependency is a good idea.

semver: I'd have to do a bit of googling as well, don't have a go to library for that. The implementation by @NHDaly in the not-yet-opened-PR seems like a good starting point. Especially if we turn all the assert statements into unit tests.

In one of the other PRs you also asked about build vs assemble scripts or some such?

(edit: semver comment updated)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Feb 26, 2019

From my side we can wait for @NHDaly to come back from vacation to get some input and give you a chance to take a look.

Things we should not lose track of:

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Feb 28, 2019

@NHDaly I copied some of the code from your branch over here. BUT, I didn't just copy the whole julia_semver.py file in, because I don't want git to credit me with writing this code when really you did all the work :) Do you want to open a PR against this branch here that adds just that one file? If the API of it stays the same, it should hopefully work with the rest of this branch here.

There is also a test failure here that I need to figure out and fix, and then all the house keeping things that @betatim pointed out.

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 2, 2019

Ha, I figured out that I can commit something but change the author info for that commit :) So, now I copied the code from @NHDaly over into this PR, and he is the author for that commit.

I removed all the asserts because they should probably become tests. We can try to sort this out once things are actually functional.

NHDaly and others added 6 commits Mar 6, 2019
Co-Authored-By: davidanthoff <anthoff@berkeley.edu>
Co-Authored-By: davidanthoff <anthoff@berkeley.edu>
Co-Authored-By: davidanthoff <anthoff@berkeley.edu>
@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 6, 2019

Any ideas why Travis is failing?

Good old bugs :) I think I found it and fixed it. Fingers crossed...

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 6, 2019

Alright everyone, thanks so much for all the feedback! I think I addressed most of them.

Tests now pass and I think the best path forward would be to merge this at this stage and then do any additional improvements in further PRs.

A couple of issues that are still open that could fall into this bucket:

  • Adding the tests for the semver code
  • Making comments more pythonic
  • Maybe some other pythonic changes to the semver code
  • Supporting older versions of Julia

Maybe someone else could try to tackle these once we have things merged?

(edit by Tim to add "old Julia versions")

@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 6, 2019

Actually, I just pushed one more commit that cleans up something :) Fingers crossed this won't change the test results :)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 6, 2019

I restarted the build as some of them failed because of timeouts. Let's see how it goes now.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 6, 2019

The remaining failed test is because the total coverage of the project will decrease if we merge this.

https://codecov.io/gh/jupyter/repo2docker/compare/e6cbf3e3a3e399508d5628b4ff1d30afed8c944f...c3d49966403b1f2b785450f86e33e49d7989e1db/diff shows which lines of the diff are covered and which aren't. I think it is basically the semver code so I'd be ok making an exception.

@willingc

This comment has been minimized.

Copy link
Member

willingc commented Mar 6, 2019

@betatim Merge away.

@davidanthoff Thank you so much. I'm so happy Julia will be better supported 🏖

@betatim betatim merged commit a9dd69a into jupyter:master Mar 6, 2019
4 of 5 checks passed
4 of 5 checks passed
codecov/project 89.03% (-1.27%) compared to e6cbf3e
Details
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 80.88% of diff hit (target 20%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 6, 2019

Done!

Thanks everyone for the efficient and enjoyable PR :)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 6, 2019

@davidanthoff do you want to deploy these changes to mybinder.org?

We typically make deployments on Tuesdays and Thursdays. We have instructions on making a new deployment that should let you do it. Ping me when you need a merge.

@davidanthoff davidanthoff deleted the davidanthoff:reorg-julia branch Mar 6, 2019
@davidanthoff

This comment has been minimized.

Copy link
Contributor Author

davidanthoff commented Mar 6, 2019

Thanks everyone for guiding me through this process and all the help :) And the vibe here is great!!

I'm wondering whether we should tackle #610 and #609 before we deploy... I think both have a very, very small potential for breaking things if folks rely on corner case behavior, so if we were to sort that out ASAP, we could probably just squeeze this in before anyone even has a chance to rely on any behavior that relates to the Project.toml. Maybe I can take a look tonight, I don't think either would be very complicated.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Mar 6, 2019

Sure thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.