Progress on spawn-pending page #1771
Conversation
|
@minrk I love this feature! |
minrk
changed the title
- add Spawner.progress method. Must be an async generator of JSON-able progress events - add /api/users/:user/server-progress eventstream endpoint - use eventstream to fill progress bar on the spawn pending page
requires calling `__aiter__` and `__anext__` instead of `async for`
allows iterating through an async generator, yielding items until another Future resolves if/when that deadline Future resolves, ready items will continue to be yielded until there is one that actually needs to wait at which point the iteration will halt
instead of server-progress
and include it in server models
simplifies "if timestamp is None" cases when we are just using it to serialize nullable timestamps to JSON
|
Very cool @minrk. |
jupyterhub/apihandlers/base.py
Outdated
| def set_default_headers(self): | ||
| super().set_default_headers() | ||
| self.set_header('Content-Type', 'application/json') | ||
| self.set_header('Content-Type', self.get_content_type()) |
There was a problem hiding this comment.
Great!
Though, is this the right place for this? I think this might be the default behaviour of tornado's set_default_headers(), where it calls self.get_content_type()? If this is overwritten in BaseHandler, this seems like behaviour that is nice enough that BaseHandler could reimplement this instead of only applying to the APIHandler base class?
For more context on where I'm coming from, If you look at the change I made in this problem, I had to make that change because setting the header directly didn't work — instead of 'text/x-yaml' it was returning 'application/octet-stream'.
There was a problem hiding this comment.
I think this might be the default behaviour of tornado's set_default_headers()
That's what I thought, too, but it's only the default behavior for StaticFileHandler and its subclasses. So I copied the tornado implementation from StaticFileHandler to our set_default_headers.
this seems like behaviour that is nice enough that BaseHandler could reimplement this
Good call. I'll move it up.
|
One question @minrk do you think that we need to display the progress circle when the bar is active? |
|
This looks great!
I actually was going to suggest a change to the current approach to the progress circle. It looks like the circle and the bar are jointly updated, when they could be performing different roles. The circle could be going at a consistent rate so long as the app is still communicating with the server (to indicate that communication is underway even if the progress bar is not yet moving). By decoupling the rate of circle change from the rate of progress bar change, you provide two separate signals: one signal that shows how far along you are and another signal that lets you know that things are underway. |
|
Great points about the spinner! The circle's a generic spinner at a constant speed, not connected to anything. I'll remove it now that there's a bar. |
|
Very cool! Will try hooking the kubespawner event reflector with this. |
no need for a spinner when we have a progress bar
so all handlers get it
asyncio.wait takes a list
They preserve error messages that are useful only delete spawners that shutdown cleanly
only terminate with iterate_until in handler, not Spawner._generate_events
function-scoped fixture for shutting down servers avoids servers leaking into neighbor tests without having to teardown the app itself after every test
consider for removal
instead of directly on asyncio
could cause spurious raises of Timeout errors
|
ok, I think this should be ready for testing. |
There was a problem hiding this comment.
Thanks @minrk. Looks great. Only thing that I'm wondering about is the API relationship between server and spawner and if server/progress calling the SpawnProgressAPIHandler is too narrow in scope if there will be other things that the server would need to measure progress for in the future. We can cross that bridge if it surfaces so I'm going to merge.
|
Should we also include a progress implementation for the SimpleLocalProcessSpawner as an easy example of where to start when extending to more complicated spawners? |
Implement progress bar on spawn-pending page:
Spawners may define a
.progressmethod. This method must be an async generator. This generator must yield event dicts of the form:{ 'progress': 80, # integer out of 100 'message': 'text', # plain-text message 'html_message': 'text', # optional html-formatted message }There is a default progress event implementation that has a single "Spawning server..." event at 50% progress.
On success, the user is sent to their server. On failure, the event log is expanded.
TODO:
cc @clkao: would you like to take a stab at implementing
Spawner.progressfor KubeSpawner and see how well this fits?