Refactor code to be more testable #22

yuvipanda · 2020-04-25T11:45:36Z

This PR refactors most of the code into three classes:

BinderUser, responsible for starting and stopping binders
NotebookClient, a standalone client for the Jupyter Notebook
REST API
Kernel, a representation of a single active kernel in a
Jupyter Notebook server.

This has many advantages:

Much easier unit testing, since everything is separated out
We can run unit tests for NotebookClient and Kernel against
a local ephemeral instance of Notebook Server, making it much
faster.
You can run multiple kernels for code off the same notebook
server, matching what the REST API does.

This PR adds unit tests, and implements functionality to shut
down binders when we are done with them (#3). We also remove
the state enum asserts in the code - they were a sanity-check
mechanism in hubtraf, and error handling should be implemented
differently here.

We should shut down binder instance when we are done, to conserve resources and prevent ourselves from hitting usage limits. Ideally, the token received from BinderHub can be used to call the JupyterHub API to shut it down. However, the token we get from BinderHub isn't a valid JupyterHub token - just a valid Jupyter Notebook token. All JupyterHub tokens are valid Jupyter Notebook tokens, but not all Jupyter Notebook tokens are valid JupyterHub tokens! This was a decision we made early on in the process so users didn't need to have 'jupyterhub' installed inside their containers. We instead kill pid 1, which stops the container. JupyterHub should garbage collect this shortly after. This is very hacky, but works Fixes pangeo-gallery#3

BinderBot class contained two bits of functionality: 1. Starting and stopping binders on a BinderHub 2. Interacting with the notebook API (start kernels, contentsmanager, etc) This commit splits them into two different classes. This has the big advantage that we can run unit tests against a local instance of jupyter notebooks, rather than starting an instance on mybinder.org all the time. Tests are much faster, and everything works as normal since it is a straight up split We added more unit tests, but lost the CLI test. Most of its functionality is covered by the other unit tests, but it would be nice to have an integration test

Each NotebookClient can now produce multiple kernels, thus matching the notebook REST API. These can be used with async context managers, so we always clean up the kernel when done. We move OperationError out to break cyclic imports

The notebook server is gone, binder can not shut down

yuvipanda · 2020-04-25T18:44:21Z

Tested the CLI with commands for https://github.com/TomAugspurger/pangeo-dask-gateway and it seemed to work.

This was present as a sanity-check mechanism in hubtraf, not needed here

rabernat

This seems great Yuvi! A huge improvement over what I had hacked together. I caught one important feature that appears to have avoided testing, which I recommend we add back.

So do you think it is not necessary to have an integration test for the CLI?

rabernat · 2020-04-27T14:43:16Z

tests/test_binderbot.py

+            await kernel.execute_notebook(
+                fname,
+                timeout=60,
+                )


I'm a little concerned here that the feature to pass environment variables is not being tested any more. It would be easy to cover this feature by adding env_vars to the call to execute_notebook.

Or maybe a separate test for that feature would be best.

@rabernat good catch. I'll add another unit test for it.

yuvipanda · 2020-04-27T14:57:28Z

@rabernat for CLIs I generally prefer Integration tests rather than unit tests. So we'll move as much of CLI functionality to functions that can be unit tested, and then write integration tests that drive the CLI.

rabernat · 2022-02-02T18:21:21Z

Yuvi, I am working on binderbot again after a long hiatus. (My goal is to fix http://gallery.pangeo.io/, which is currently broken due to Pangeo Binder being down.)

I'd like to get this PR merged. My plan is to just resolve the conflicts manually and keep your changes. But before doing that I thought I'd ping you again since it has been almost 2 years! 🙃

yuvipanda · 2022-02-03T17:33:27Z

@rabernat wow, it has really been two years. Damn. Yes, please do what you can :)

weiji14

Not sure if there's still appetite to revive binderbot, but here's a few suggestions to maybe nudge things along 🙂

Context is that I'd be keen to see #35 fixed, which is partly an issue on 1) handling changes to the Binder API that is returned different HTML responses compared to a year or two ago and 2) dealing with oauth tokens required for the aws-uswest2 Pangeo Binder. This PR will probably only deal with (1) if I'm not mistaken.

weiji14 · 2022-11-19T04:36:50Z

binderbot/binderbot.py

@@ -54,7 +35,6 @@ def __init__(self, binder_url, repo, ref):
        self.binder_url = URL(binder_url)
        self.repo = repo
        self.ref = ref
-        self.state = BinderUser.States.CLEAR
        self.log = logger.bind()

    async def start_binder(self, timeout=3000, spawn_refresh_time=20):


At L45 below, need to change build/gh to v2/gh.

weiji14 · 2022-11-19T04:47:30Z

binderbot/binderbot.py

@@ -90,215 +70,21 @@ async def start_binder(self, timeout=3000, spawn_refresh_time=20):
                self.log.msg(f'Binder: Waiting on event stream (phase: {phase})', action='binder-start', phase='event-stream')


Suggest checking that the notebook_url and token variables are set before proceeding to the next piece of logic. This will help a little bit with debugging #35.

Suggested change

self.log.msg(f'Binder: Waiting on event stream (phase: {phase})', action='binder-start', phase='event-stream')

self.log.msg(f'Binder: Waiting on event stream (phase: {phase})', action='binder-start', phase='event-stream')

try:

self.notebook_url

self.token

except AttributeError as e:

raise OperationError from e

weiji14 · 2022-11-19T04:53:26Z

binderbot/cli.py

@@ -8,6 +8,7 @@
 import nbformat

 from .binderbot import BinderUser
+from .notebookclient import NotebookClient

 # https://github.com/pallets/click/issues/85#issuecomment-43378930
 def coro(f):


The default binder URL https://binder.pangeo.io at L22 below doesn't work anymore. Point to https://aws-uswest2-binder.pangeo.io instead? Or wait for a new Binder deployment from 2i2c-org/infrastructure#919 🙂

weiji14 · 2022-11-19T04:58:38Z

binderbot/binderbot.py

@@ -54,7 +35,6 @@ def __init__(self, binder_url, repo, ref):
        self.binder_url = URL(binder_url)
        self.repo = repo
        self.ref = ref
-        self.state = BinderUser.States.CLEAR
        self.log = logger.bind()

    async def start_binder(self, timeout=3000, spawn_refresh_time=20):


At L54 below, I'm not quite sure if the if line.startswith('data:'): statement works anymore. The HTML response I'm getting when testing with https://mybinder.org looks like:

<meta id="badge-base-url" data-url="https://mybinder.org/"> <meta id="build-token" data-token="eyJhbGciOiJIUzI1NiIsInR5cCI6IkpxVCJ9.eyJleHAiOjE2Njg4MzA3MTksImF1ZCI6ImdoL2dlby1zbWFydC9kZWVwaWNlZHJhaW5waXBlL3RlbXAtaXB5bmIiLCJvcmlnaW4iOiJteWJpbmRlci5vcmcifQ.pALJ9hVdHdq7-YbgIYkMU2lcdeu7MphArjnw38_DbKc">

so maybe the if-statement should be finding data-url and data-token instead?

yuvipanda added 2 commits April 25, 2020 11:39

yuvipanda marked this pull request as ready for review April 25, 2020 11:45

Split kernel operations into its own class

37f7abf

Each NotebookClient can now produce multiple kernels, thus matching the notebook REST API. These can be used with async context managers, so we always clean up the kernel when done. We move OperationError out to break cyclic imports

yuvipanda force-pushed the feat/notebookclient branch from 13ac3d4 to 37f7abf Compare April 25, 2020 18:11

Stop trying to stop a kernel after binder is shut down

4a3bded

The notebook server is gone, binder can not shut down

yuvipanda changed the title ~~[WIP] Refactor~~ Refactor code to be more testable Apr 25, 2020

Split NotebookClient to its own file

094c5a0

yuvipanda requested review from rabernat and TomAugspurger and removed request for rabernat April 25, 2020 18:44

Remove state enum

84d1d14

This was present as a sanity-check mechanism in hubtraf, not needed here

yuvipanda requested a review from rabernat April 25, 2020 18:46

rabernat requested changes Apr 27, 2020

View reviewed changes

weiji14 reviewed Nov 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor code to be more testable #22

Refactor code to be more testable #22

yuvipanda commented Apr 25, 2020 •

edited

Loading

yuvipanda commented Apr 25, 2020

rabernat left a comment

rabernat Apr 27, 2020

yuvipanda Apr 27, 2020

yuvipanda commented Apr 27, 2020

rabernat commented Feb 2, 2022

yuvipanda commented Feb 3, 2022

weiji14 left a comment

weiji14 Nov 19, 2022

weiji14 Nov 19, 2022

weiji14 Nov 19, 2022

weiji14 Nov 19, 2022

		@@ -90,215 +70,21 @@ async def start_binder(self, timeout=3000, spawn_refresh_time=20):
		self.log.msg(f'Binder: Waiting on event stream (phase: {phase})', action='binder-start', phase='event-stream')

-                self.log.msg(f'Binder: Waiting on event stream (phase: {phase})', action='binder-start', phase='event-stream')
+                self.log.msg(f'Binder: Waiting on event stream (phase: {phase})', action='binder-start', phase='event-stream')
+        try:
+            self.notebook_url
+            self.token
+        except AttributeError as e:
+            raise OperationError from e

Refactor code to be more testable #22

Are you sure you want to change the base?

Refactor code to be more testable #22

Conversation

yuvipanda commented Apr 25, 2020 • edited Loading

yuvipanda commented Apr 25, 2020

rabernat left a comment

Choose a reason for hiding this comment

rabernat Apr 27, 2020

Choose a reason for hiding this comment

yuvipanda Apr 27, 2020

Choose a reason for hiding this comment

yuvipanda commented Apr 27, 2020

rabernat commented Feb 2, 2022

yuvipanda commented Feb 3, 2022

weiji14 left a comment

Choose a reason for hiding this comment

weiji14 Nov 19, 2022

Choose a reason for hiding this comment

weiji14 Nov 19, 2022

Choose a reason for hiding this comment

weiji14 Nov 19, 2022

Choose a reason for hiding this comment

weiji14 Nov 19, 2022

Choose a reason for hiding this comment

yuvipanda commented Apr 25, 2020 •

edited

Loading