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] Bump notebook package version #628

Merged
merged 5 commits into from Mar 24, 2019

Conversation

@betatim
Copy link
Member

betatim commented Mar 23, 2019

This updated the version of the notebook package and others. The freeze.py script wouldn't run because of some path gubbins so I've updated it as well.

Fixes #627
Fixes #626

Copy link
Collaborator

choldgraf left a comment

A few clarifying questions - in general it LGTM, if we have clear answers then I'm +1 on merge

repo2docker/buildpacks/conda/environment.frozen.yml Outdated Show resolved Hide resolved
repo2docker/buildpacks/conda/environment.py-3.6.yml Outdated Show resolved Hide resolved
repo2docker/buildpacks/conda/freeze.py Outdated Show resolved Hide resolved
@betatim betatim changed the title [WIP] Bump notebook package version [MRG] Bump notebook package version Mar 23, 2019
@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented Mar 23, 2019

As far as I can tell from testing locally there is a notebook package for Python 3.5 and 3.7 that works. I also built binder-examples/conda and binder-examples/requirements which worked and notebooks launched.

Let's see if travis is happy as well.

@betatim betatim force-pushed the betatim:bump-notebook2 branch from ecb4f28 to 4a9f4a5 Mar 23, 2019
@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented Mar 23, 2019

sounds good - if travis is happy then I am happy (seems reasonable to bump the other dependencies too, just wasn't sure if it was a requirement)

@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented Mar 23, 2019

Travis fails because the new notebook gives an exit code of 137 when it gets killed. When we try to stop a container we first send a SIGTERM and then if that didn't work SIGKILL. The reason we see an exit code of 137 is because the notebook didn't shutdown properly on SIGTERM and the ngot killed by SIGKILL. Investigating what the proper fix is.

For some reason the notebook server doesn't exit when we send the
SIGTERM signal to the container and as a result waiting for the
container to shutdown leads to an exit code of 137 (the notebook server
gets kill by SIGKILL).
@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented Mar 23, 2019

This updates the test so that it shutsdown the container but doesn't care how it exits.

I don't quite understand why the container doesn't stop when we ask it to stop but needs killing :-/

@choldgraf

This comment has been minimized.

Copy link
Collaborator

choldgraf commented Mar 23, 2019

I am....uncertain whether that's a problem or not :-/ maybe @minrk or @yuvipanda could give their opinion on whether this is a bad idea? Otherwise I'm still +1 on the PR

@betatim betatim merged commit f18835f into jupyter:master Mar 24, 2019
5 checks passed
5 checks passed
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 20% of diff hit (target 20%)
Details
codecov/project 90.75% remains the same compared to e921993
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@betatim

This comment has been minimized.

Copy link
Member Author

betatim commented Mar 24, 2019

I think it is weird and needs looking at but it isn't "dangerous". It seems if you do the equivalent sequence via docker run, docker exec, docker stop it works as expected.

I'll merge now so we can deploy this on Sunday morning (now) as I think we should bump the build prefix which causes a lot of rebuilds and load -> would be good to start that at a quiet time of the week.

@betatim betatim deleted the betatim:bump-notebook2 branch Mar 24, 2019
@minrk

This comment has been minimized.

Copy link
Member

minrk commented Mar 25, 2019

If the test is passed by the time we kill it, then that should be fine. I think we only care about early exits.

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