jupyterlab icon indicating copy to clipboard operation
jupyterlab copied to clipboard

Add events service

Open afshin opened this issue 3 years ago • 5 comments
trafficstars

work in progress

This PR adds support for the newly added /api/events endpoints in jupyter_server

References

https://github.com/jupyter-server/jupyter_server/issues/780

Code changes

An earlier version of this PR added some new concepts but they will create more confusion than benefit. We should revisit the idea of how to swap out the ServiceManager from an extension in another PR.

  • ~~ServiceManager.getInstance(): ServiceManager.IManager | null You get either the currently existing service manager or null.~~
  • ~~ServiceManager.setInstance(manager: ServiceManager.IManager): ServiceManager.IManager You can set the service manager instance (and trigger any already existing one to dispose()~~
  • ~~Since the ServiceManager.IManager interface is an observable disposable, this allows the JupyterFrontEnd to discover that its service manager has been disposed and then set it to a new service manager~~

User-facing changes

Backwards-incompatible changes

afshin avatar Jun 07 '22 22:06 afshin

Thanks for making a pull request to jupyterlab! To try out this branch on binder, follow this link: Binder

jupyterlab-probot[bot] avatar Jun 07 '22 22:06 jupyterlab-probot[bot]

Adds a static method ServiceManager.getInstance() that returns a ServiceManager if one has already been instantiated. This allows an extension to directly import a ServiceManager instance without threading it through each component that resides in the stack between where an extension was activated and where the component in question resides.

Does that mean we will expect extensions and plugins to use ServiceManager.getInstance() to access the service manager? And do we encourage this practice, or just provide an alternative?

Also wondering how this would work when a custom ServiceManager is provided to the JupyterLab application (or to a different JupyterFrontEnd). Would calling ServiceManager.getInstance() return the custom service manager instance?

jtpio avatar Jun 08 '22 13:06 jtpio

@jtpio After thinking it through and discussing it with @blink1073, I think a better model is the following (I'll update the top-level comment as well):

  • ServiceManager.getInstance(): ServiceManager.IManager | null You get either the currently existing service manager or null.
  • ServiceManager.setInstance(manager: ServiceManager.IManager): ServiceManager.IManager You can set the service manager instance (and trigger any already existing one to dispose()
  • Since the ServiceManager.IManager interface is an observable disposable, this allows the JupyterFrontEnd to discover that its service manager has been disposed and then set it to a new service manager:
    this._serviceManager =
      options.serviceManager ||
      ServiceManager.getInstance() ||
      ServiceManager.setInstance(new ServiceManager());
    this._serviceManager.disposed.connect(() => {
      this._serviceManager =
        ServiceManager.getInstance() ||
        ServiceManager.setInstance(new ServiceManager());
    });
    

afshin avatar Jun 08 '22 14:06 afshin

Does that mean we will expect extensions and plugins to use ServiceManager.getInstance() to access the service manager? And do we encourage this practice, or just provide an alternative?

This is just another alternative to try to simplify the structure of some components and not require them to thread down the JupyterFrontEnd#serviceManager but it isn't prescriptive about how somebody should get a service manager.

afshin avatar Jun 08 '22 14:06 afshin

Also wondering how this would work when a custom ServiceManager is provided to the JupyterLab application (or to a different JupyterFrontEnd). Would calling ServiceManager.getInstance() return the custom service manager instance?

Yes, in this model it would return the custom one. The idea is that perhaps an extension might want to globally replace the service manager the application uses.

afshin avatar Jun 08 '22 15:06 afshin

This PR adds support for the newly added api/events endpoints in jupyter_server in @jupyterlab/services.

Should this require a bump to the jupyter_server dependency lower bound?

EDIT: ah this already seems to be the case on the latest commit :+1:

https://github.com/jupyterlab/jupyterlab/blob/4847b1b1df6e1e64a9411f0e78291876a137e6a1/pyproject.toml#L44

jtpio avatar Nov 15 '22 15:11 jtpio

EDIT: ah this already seems to be the case on the latest commit +1

Since this is already the case the following TODO can probably be marked as completed?

  • [ ] Remove capability probing for api/events in EventManager#_subscribe() after jupyter_server 2 becomes a hard requirement.

jtpio avatar Nov 15 '22 15:11 jtpio

This version of the PR can be merged into the 3.6 branch, but a follow-on PR should remove the capability probing block and should also remove the Private.Stream implementation.

On Tue, 15 Nov 2022 at 15:16 Jeremy Tuloup @.***> wrote:

@.**** commented on this pull request.

In packages/services/src/event/index.ts https://github.com/jupyterlab/jupyterlab/pull/12667#discussion_r1022921252 :

  • const response = await makeRequest(url, init, serverSettings);
  • if (response.status !== 204) {
  •  throw new ResponseError(response);
    
  • }
  • }
  • /**
    • Subscribe to event bus emissions.
  • */
  • private async _subscribe(): Promise {
  • if (this.isDisposed) {
  •  return;
    
  • }
  • // TODO: (1) Make the test emission a request with an appropriate schema.
  • // TODO: (2) Remove try block when jupyter_server 2 is a hard requirement.

Looks like this can already be done now since the current master has that hard requirement:

https://github.com/jupyterlab/jupyterlab/blob/4847b1b1df6e1e64a9411f0e78291876a137e6a1/pyproject.toml#L44

— Reply to this email directly, view it on GitHub https://github.com/jupyterlab/jupyterlab/pull/12667#pullrequestreview-1181061705, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABG6KIYVIOLCQGYADMAOZ3WIOSNVANCNFSM5YEOOLRQ . You are receiving this because you were assigned.Message ID: @.***>

afshin avatar Nov 16 '22 07:11 afshin

ah ok, adding to the 3.6 milestone then if it was meant to be backported.

jtpio avatar Nov 16 '22 07:11 jtpio

Although the 3.6 branch does not include the changes from https://github.com/jupyterlab/jupyterlab/pull/11537.

So it's not clear whether it can still be backward compatible because that will likely require changing the ServiceManager.IManager interface.

jtpio avatar Nov 16 '22 07:11 jtpio

Although the 3.6 branch does not include the changes from https://github.com/jupyterlab/jupyterlab/pull/11537.

So it's not clear whether it can still be backward compatible because that will likely require changing the ServiceManager.IManager interface.

The events manager can be added as optional in the interface in 3.6.x. so it stays backward compatible.

fcollonval avatar Nov 16 '22 09:11 fcollonval

Based on the discussions above here are a couple of follow-ups to track in separate issues and PRs:

  • [ ] In EventManager#_subscribe(), make the test emission a request with a registered schema. This requires registering a schema with the back-end application's event bus.
  • [ ] Remove capability probing for api/events in EventManager#_subscribe() after jupyter_server 2 becomes a hard requirement.
  • [ ] Write tests for EventManager in the @jupyterlab/services test suite.
  • [ ] Make events optional when backporting to 3.6.x

jtpio avatar Nov 17 '22 09:11 jtpio

Maybe making EventManager optional should be in this PR since it's basically already got outstanding tasks to make it 4.0-ready.

afshin avatar Nov 17 '22 11:11 afshin

@fcollonval Thank you for helping with the test failures!

I added a couple commits that switch to checking PageConfig for the server version instead of making a request (cc: @blink1073 @Zsailer) and I switched to using a Poll instance to manage backing off when socket connections fail so that we don't flood re-connection attempts.

afshin avatar Nov 21 '22 11:11 afshin

@meeseeksdev please backport to 3.6.x

fcollonval avatar Nov 21 '22 16:11 fcollonval

Oops, something went wrong applying the patch ... Please have a look at my logs.

lumberbot-app[bot] avatar Nov 21 '22 16:11 lumberbot-app[bot]