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

[CIVP-18964] Update Notebook Package Version #50

Merged
merged 7 commits into from
Sep 3, 2019

Conversation

aehrmann
Copy link
Contributor

This PR updates the notebook package version to 5.7.8 for Python 2 and to 6.0.0 for Python 3 (in order to fix, among other things, the 500 error when trying to download a notebook as an ipynb file)

@aehrmann aehrmann requested a review from leanne73 August 28, 2019 16:24
@aehrmann aehrmann changed the title Civp 18964 update notebook package version [CIVP-18964] Update Notebook Package Version Aug 28, 2019
# for versions of jupyter before 5.7.5 we need to specify tornado<6
# as these older versions of jupyter will break with tornado 6
notebook==6.0.0 ; python_version >= '3.5'
notebook==5.7.8 ; python_version < '3.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

So why the two different versions now?

Copy link
Contributor Author

@aehrmann aehrmann Aug 28, 2019

Choose a reason for hiding this comment

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

According to the notebook package changelog, the 6.0.0 release fixes the issue with the 500 error on downloading a notebook in ipynb format. I can't direct link the line, but in the notes for 6.0.0 it mentions "Respect nbconvert entrypoints as sources for exporters", which was causing the 500.

Apparently nbconvert in 5.7.8 works fine for Python2 ¯_(ツ)_/¯

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh, so fixing the 500 error requires 6.0.0 on python3? Interesting. Is there any reason to keep python2 on 5.7.8? If we're bumping one, it seems like we might as well bump the other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIRC there was an issue with python 2 and 6.0.0 but I’ll double check just to make sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They actually dropped support for python2 in 6.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

😆 😭 Wonder when we'll be able to drop support for it... Can we add a comment explaining why 5.7.8 is needed for python2? Otherwise I think future us may keep trying to upgrade it.

@@ -28,7 +28,7 @@ def find_and_install_requirements(requirements_path, c):

def config_jupyter(c):
# Jupyter Configuration
c.NotebookApp.ip = '*'
c.NotebookApp.ip = '0.0.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the reasons for/consequences of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is necessary to fix this error when starting the notebook image: ValueError: '' does not appear to be an IPv4 or IPv6 address (this bug is referenced in the JIRA ticket)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, we've run into that error before, but there was supposed to be a fix introduced in 5.7.8. Does that fix not actually work, or is it not included in 6.0.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the fix completely works. On the python2 notebook, when I change the value back to *, I get a different error:

socket.gaierror: [Errno -2] Name or service not known

The change above fixes this for python2 and doesn't seem to affect the other notebooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, that's too bad. 😞 Okay then.

@aehrmann
Copy link
Contributor Author

@leanne73 Let me know if I answered your questions or if you need some clarification, either here or offline. Thanks!

@aehrmann
Copy link
Contributor Author

aehrmann commented Sep 3, 2019

@leanne73 Does this PR look ready for release?

Copy link
Contributor

@leanne73 leanne73 left a comment

Choose a reason for hiding this comment

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

Looks pretty good - one request though: Can we add a comment explaining why 5.7.8 is needed for python2? (i.e. that 6.0.0 drops support for python 2) Otherwise I think future us may keep trying to upgrade it.

leanne73
leanne73 previously approved these changes Sep 3, 2019
Copy link
Contributor

@leanne73 leanne73 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@leanne73 leanne73 left a comment

Choose a reason for hiding this comment

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

LGTM

@aehrmann aehrmann merged commit 25ded59 into master Sep 3, 2019
@aehrmann aehrmann deleted the CIVP-18964-update-notebook-package-version branch September 3, 2019 20:49
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.

2 participants