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

Ensure stdout works in Jupyter Notebooks #121

Merged
merged 3 commits into from
Aug 20, 2020
Merged

Conversation

manics
Copy link
Member

@manics manics commented Aug 19, 2020

sys.stdout in Jupyter notebooks may not have a buffer attribute.

To test: run create_companion(images=[companion]) (i.e. use the default out parameter) in a Jupyter Notebook.

See e.g. Yelp/mrjob#1443

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

Changes makes sense. Just one explanatory line about the context of the change would be useful. Otherwise LGTM.

Outside the scope of this PR, do you know of some mechanism allowing to test minimal Jupyter notebooks in a tox environment?

@@ -6,11 +6,6 @@
import uuid
import xml.etree.ElementTree as ET

if sys.version_info[0] > 2:
Copy link
Member

Choose a reason for hiding this comment

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

👍 especially as setup.py forces Python3+

out = sys.stdout.buffer
elif not out:
out = sys.stdout
except AttributeError:
Copy link
Member

Choose a reason for hiding this comment

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

Happy with either the try/catch approach or the getattr one-liner.

Is it possible to add a comment similar to Yelp/mrjob#1443 explaining the special handling? Is there a reference page explaining why the stdout is different in the Jupyter notebook environment?

Copy link
Member Author

@manics manics Aug 20, 2020

Choose a reason for hiding this comment

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

Updated. I'm not aware of an explanation, my best guess is it was originally written to match python2 sys.stdout and no one thought of adding .buffer to it.

@manics
Copy link
Member Author

manics commented Aug 20, 2020

For testing you can use nbval, for example
https://github.com/IDR/idr-notebooks/blob/master/docker/test_notebooks.sh
though note this script isn't yet automatically run in travis on that repo.

@sbesson sbesson merged commit 1d0d6ee into ome:master Aug 20, 2020
@manics manics deleted the stdout branch August 20, 2020 10:50
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