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

Review API JupyterServerProvider and prepare for stable API #13936

Closed
wants to merge 21 commits into from

Conversation

DonJayamanne
Copy link
Contributor

@DonJayamanne DonJayamanne commented Jul 18, 2023

For #13894

@DonJayamanne DonJayamanne changed the title Review Proposed API Review API JupyterServerProvider and prepare for stable API #13894 Jul 18, 2023
@DonJayamanne DonJayamanne changed the title Review API JupyterServerProvider and prepare for stable API #13894 Review API JupyterServerProvider and prepare for stable API Jul 18, 2023
src/proposed.api.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mjbvz mjbvz left a comment

Choose a reason for hiding this comment

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

@DonJayamanne Can you share a code example of how you'd use this API?

My overall impression is that the flow of the API seems a little strange. Typically APIs like this fit into one of two models:

  • A pull model. Callers register providers that the host then calls
  • A push model. Callers call functions to modify a managed collection

This seems closer to the push model but I don't currently understand why all the create* functions are needed. It would help to know which objects are meant to be manage/live and which ones are simply meant to contain data

I'd also be interested to see if we could use a provider model instead as this gives the host more control

src/proposed.api.d.ts Outdated Show resolved Hide resolved
src/proposed.api.d.ts Outdated Show resolved Hide resolved
src/proposed.api.d.ts Outdated Show resolved Hide resolved
* @return {*} {JupyterServerCollection}
* @memberof JupyterAPI
*/
createServerCollection(id: string, label: string): Promise<JupyterServerCollection>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are collections scoped to the extension? For instance, what happens if two extensions use the same id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they are scoped to the extension,
I discussed with Peng the old code requires it to be unique (i.e. if another extension uses the same id, then that could result in problems), but I guess thats a problem that we need to fix.

So, lets go with unique per extension, which is my preference.
However, the challenge here is we need to know the caller (extension), today we get this information by analyzing the stack trace.

src/proposed.api.d.ts Outdated Show resolved Hide resolved
@DonJayamanne
Copy link
Contributor Author

DonJayamanne commented Jul 20, 2023

@mjbvz

  • I have turned all claases into interfaces
  • Here are some screenshots and link to the sample extension

Here is the sample repo using the new API
https://github.com/DonJayamanne/vscode-jupyter-launcher
You would be interested in the JupyterInteration.ts file https://github.com/DonJayamanne/vscode-jupyter-launcher/blob/main/src/jupyterIntegration.ts

If you look at the commit history, the first commit shows how the old API is used (lot more complex,. and used a provider model and had access to qujick picks as well, no real domain model)

1. This is the standard kernel picker (built by VS Code)

Screenshot 2023-07-20 at 13 27 35

2. From previous quick pick user selects the option Select Another kernel (this is provided by vscode)

The list of items here is contributed by extensions.
Since other extensions can contribute Jupyter Servers to the Jupyter extension they contribute their collection with us, and we add an entry into this VS Code list
So an entry in this item is called a collection, as each item can in turn have multiple servers (see asmples befolow).
Here the second from last Local Jupyter Server is contributed to the Jupyter extension via the api createServerCollection('Local Jupyter Server')

Screenshot 2023-07-20 at 13 27 41

3. Once the user selects the Local Jupyter Server, we get to this screen.

At this point jupyter extension will look for servers contributed by the 3rd party extension, if there are we display them.
However for the first time its unlikely a server would exist, hence the 3rd party extension needs a way to get the user to start/select a server.
For this they can add items into the quick pick that create the servers.
This is done by 3d party calling the api as follows

collection.createServerCreationItem('', 'Start New Jupyter Lab Server, ()=> {....});
collection.createServerCreationItem('', 'Start New Jupyter Notebook Server, ()=> {....});

If there is only one item in the list, the 3rd party extension can mark that has picked and we (Jupyter extension) will bypass displaying this list and automatically select that first item.
See below, we have dispalyed the two items contributed by the 3rd party

Screenshot 2023-07-20 at 13 27 47

4. Upon selecting on of the above items the Jupyter extension calls the callback

The 3rd party handles the callback, and displays their own UI (AzML, Synapse and other extensions do this today, and display their own quick picks).
In this sample I have a custom quick pick with options (this is from 3rd party extension)
The UI can be as complex as the 3rd party needs it to be (AzML has 3 steps in their own quick pick)
So here once we select something we start a Jupyter server
The 3rd party extension will then call collection.createServer(<Display Name>) and return that server from the callback.

If the Jupyter extension gets a server retrurned from the callback, we know that a server was created and we display that server in the next quick pick and we display the list of the kernels.

Screenshot 2023-07-20 at 13 27 58

5. Here the Jupyter extension got the server via the callback and we now dispaly that server and its kernels

Screenshot 2023-07-20 at 13 29 08

6. If we hit the back buttoon, then we get to the same screen but this time we have the servers listed along with the creation items

At this point the 3rd party extension can optionally remove the creation items if they want
They can dispose the Start Local Jupyter Server as they know one has been started (after all 3rd party extensions have created the server via the api call)
Hence the reason to expose the dispose on JupyterServerCreationItem` (this way they can add/remove anytime)

If you were to go back to the first step in the kernel picker and select Local Jupyter Server then you get the same list of items

Screenshot 2023-07-20 at 13 28 22

7. Multiple servers (created via createServer)

Screenshot 2023-07-20 at 16 05 43

@DonJayamanne DonJayamanne force-pushed the issue13894Review branch 2 times, most recently from 53abe3b to 01c3fd8 Compare July 20, 2023 09:29
@mjbvz
Copy link
Contributor

mjbvz commented Jul 20, 2023

Thanks @DonJayamanne! Based on your description of the flow, it seems like a pull model may be a better fit. Have you consider a more "provider like" approach?

@DonJayamanne
Copy link
Contributor Author

@mjbvz thanks, we did have a provider style API earlier (with quick picks), will try to get that again and pass it your way.
thanks again for your feedback.

@DonJayamanne
Copy link
Contributor Author

hi @mjbvz we decided to go with a push model as the UI and requirements are similar to notebook controllers.
In Notebook controllers we had a pull (provider) model and then moved to a push model, going with the same here.

Hence prefer to go with the push, however I have updated this PR by adding another file with the pull (provider) model API. See here https://github.com/microsoft/vscode-jupyter/blob/86da8624cbff9a9cee0844642b848bd4637472bf/src/api.proposed.d.ts

Will bring this to the next API discussion.

@DonJayamanne
Copy link
Contributor Author

Closing this PR as most of the issues ahve been addressed.

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