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

adding a binder example #7

Merged
merged 7 commits into from
Mar 10, 2020
Merged

adding a binder example #7

merged 7 commits into from
Mar 10, 2020

Conversation

choldgraf
Copy link
Contributor

I decided to add a little demo notebook as a Binder example to show how the API works. Here's a link to the version that's on my branch:

https://mybinder.org/v2/gh/choldgraf/nbclient/binder?filepath=binder%2Frun_nbclient.ipynb

I am having a hard time launching the Binder instance, which is confusing to me. It builds fine and then just hangs when it gets to the "launching server" part.

I noticed that in the build logs, when it got to the scrapbook install there was this warning:

ERROR: jupyter-client 5.3.4 has requirement jupyter-core>=4.6.0, but you'll have jupyter-core 4.4.0 which is incompatible.
ERROR: nbclient 0.0.0 has requirement nbformat>=5.0, but you'll have nbformat 4.4.0 which is incompatible.

Could this be an issue with scrapbook dependencies?

To demo **NBClient** interactively, click the Binder link below:

.. image:: https://mybinder.org/badge_logo.svg
:target: https://mybinder.org/v2/gh/jupyter/nbclient/master?filepath=binder%2Frun_nbclient.ipynb
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: these won't work until the PR gets merged

Copy link
Contributor

Choose a reason for hiding this comment

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

All good!

@MSeal
Copy link
Contributor

MSeal commented Feb 4, 2020

Could this be an issue with scrapbook dependencies?

Hmm no scrapbook doesn't have any such dependency pins in it's latest (old at this point) release: https://github.com/nteract/scrapbook/blob/d3cb3f126b507ae75dee4b51df958e3db47aeb0c/requirements.txt

I don't see an obvious reason that error would occur, other than default installs on the binder server. I'm trying to see the error message myself but binder isn't booting up for me :/ If the binder notebook still ran let's just plan to merge and we can look more into it later.

Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

Might want to add something like: https://github.com/nteract/papermill/blob/master/tox.ini#L28-L33 but with a python -c 'from nbclient.execute import executenb; executenb(<nb_path>)' instead of a papermill call

@choldgraf
Copy link
Contributor Author

OK, added a tox example and updated the notebook code to use the execute function since that seems to be more stable than using the Executor -> NotebookClient class...I also tried it on Binder and it loaded properly this time! So if tests are happy then I think we are good to go 👍

@choldgraf
Copy link
Contributor Author

choldgraf commented Feb 21, 2020

mmmk - two things:

  • Running this code on binder returns an error that scrapbook doesn't have a glue attribute...maybe this is because it's been a while since scrapbook has been released?
  CellExecutionError: An error occurred while executing the following cell:
------------------
# Use scrapbook to store this data in the notebook
sb.glue('dataframe', data.to_dict())
------------------

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-3-535486294a30> in <module>
      1 # Use scrapbook to store this data in the notebook
----> 2 sb.glue('dataframe', data.to_dict())

AttributeError: module 'scrapbook' has no attribute 'glue'
  • There are weird travis errors about manifests that I don't understand...could somebody explain them to me, or tell me how to fix them? :-)

@MSeal
Copy link
Contributor

MSeal commented Feb 25, 2020

That's odd -- every version of scrapbook has had the glue function. I can look into that later.

As for the manifest check, that's a check to make sure the wheel build will be complete and correct. It's a little tedious to manage but adding the check makes for far fewer bad releases. Add the following to the MANIFEST.in file:

exclude binder
recursive-exclude binder *.ipynb
recursive-exclude binder *.txt

as the binder code doesn't need to be in wheels.

@choldgraf
Copy link
Contributor Author

lol I'm an idiot - I think it's because I put scrapbook in the requirements.txt file instead of nteract-scrapbook.

That said, I have another weird error with the execution loop. on Binder and locally, when I run

# We use nbformat to represent our empty notebook in-memory
nb = nbf.read('./empty_notebook.ipynb', nbf.NO_CONVERT)

# Execute our in-memory notebook, which will now have outputs
nb = nbclient.execute(nb)

I get:

/srv/conda/envs/notebook/lib/python3.7/asyncio/base_events.py in run_forever(self)
    523         self._check_closed()
    524         if self.is_running():
--> 525             raise RuntimeError('This event loop is already running')
    526         if events._get_running_loop() is not None:
    527             raise RuntimeError(

RuntimeError: This event loop is already running

@choldgraf
Copy link
Contributor Author

Maybe @davidbrochart has an idea since he added the async stuff?

@choldgraf choldgraf mentioned this pull request Feb 28, 2020
@MSeal MSeal added this to the 0.2.0 milestone Mar 8, 2020
@MSeal
Copy link
Contributor

MSeal commented Mar 9, 2020

Hmm having a hard time reproducing the issues. Try this branch against #34 and see if you still are getting the error? I fixed a few things in that PR that could cause the error above in theory.

@choldgraf
Copy link
Contributor Author

well super annoyingly, I can no-longer recreate this locally either. Though the binder link in the top comment results in this error: RuntimeError: This event loop is already running. That sounds like it might be related to #34 ? I'd be fine w/ rebasing on master once that is merged and seeing if that fixes the Binder error...

@davidbrochart
Copy link
Member

Sorry @choldgraf for replying so late. I opened a PR to fix the issue in #35.

@choldgraf
Copy link
Contributor Author

sounds great - when that's merged, I'll rebase on master and we'll see what happens!

@choldgraf
Copy link
Contributor Author

rebased on master (and fixed up some installation requirements), and it now works on Binder! This is RTG from my perspective unless folks have comments 👍

@davidbrochart
Copy link
Member

👍

Copy link
Contributor

@MSeal MSeal left a comment

Choose a reason for hiding this comment

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

Try that suggested change and see if it fixes the dist built

setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
choldgraf and others added 3 commits March 9, 2020 13:21
@choldgraf
Copy link
Contributor Author

fair enough - no reason to introduce extra complexity then, I'll keep it to your suggestion. I may be mistaken, but you replaced a read_reqs with read...I think that was unintentional? (latest commit un-does this)

@MSeal
Copy link
Contributor

MSeal commented Mar 9, 2020

fair enough - no reason to introduce extra complexity then, I'll keep it to your suggestion. I may be mistaken, but you replaced a read_reqs with read...I think that was unintentional? (latest commit un-does this)

Yeah my bad, meant read_reqs

@MSeal
Copy link
Contributor

MSeal commented Mar 9, 2020

I can help debug this more tonight and get it merged if it helps -- looks like my suggestion wasn't the root cause.

@choldgraf
Copy link
Contributor Author

Sounds good - maybe it has something to do with the fact that binder/requirements.txt are only in the testenv:binder part of tox.ini, even though it's required in setup.py? (maybe it's overkill and this doesn't need to be in setup.py?)

@MSeal
Copy link
Contributor

MSeal commented Mar 10, 2020

Yeah the issue was that binder was excluded from the manifest but setup.py tried to read it to set the binder extra reqs. Since we don't need setuptools to manage binder tests I removed it as an extra_arg to fix the issue.

@MSeal MSeal merged commit 96148fd into jupyter:master Mar 10, 2020
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.

3 participants