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

Add notebook controller implementation #980

Merged
merged 108 commits into from
Jun 29, 2021
Merged

Add notebook controller implementation #980

merged 108 commits into from
Jun 29, 2021

Conversation

davidanthoff
Copy link
Member

@davidanthoff davidanthoff commented Feb 11, 2020

This PR has only a kernel implementation that can be used for Jupyter notebooks. The Jupyter extension provides all the file IO functionality, and we are only in charge of executing the code and returning results.

List of things that we still need to resolve on our end before we can ship a first version:

  • Support different Julia versions as kernels
  • Return MIME bundles rather than just one representation for results, like IJulia does
  • Add kernel lifetime management UI
  • Remove the use of one terminal per kernel, instead us child processes directly
  • Fix close/dispose/shutdown situation, right now a lot of stuff will just hang around

- [ ] What happens if a notebook is closed, kernel is still running, and the same file is opened again? The kernel should probably be reused?
- [ ] Check that all the MIME types we support in IJulia also work
- [ ] Figure out what we do about variable viewers. Could we hook the button in the toolbar to view the Julia workspace?
- [ ] Is the Julia extension going to be activated every time someone opens a Jupyter notebook, even if no Julia is used in that notebook? We might have to make sure that our extension does almost nothing in that scenario
- [ ] Generally go through the entire IJulia codebase and check whether there are features we need to have here as well
- [ ] Think about kernel status UI. JupyterLab has nice info like "Kernel idle", "Kernel starting" etc. Not sure where we could have that.
- [ ] Handle Julia environments

List of things that need to be resolved on the VS Code/Jupyter extension end before we can ship:
- [ ] I think general kernel management, although I might be wrong. Not clear to me right now whether the different kernels we register will now be automatically selected properly and whether kernelspec data is properly written to Jupyter notebook files.

  • Are things stable enough in VS Code and the Jupyter extension that we could ship this PR? My sense is yes, but would be good enough to double check.

UPDATE: Moved a lot of small issues into separate proper issues, we can resolve these after this PR has been merged.

@davidanthoff
Copy link
Member Author

Did we want to also add a placeholder entry if user doesn't have Julia installed.

Yes, that sounds like a good idea! I guess we might think about this scenario outside of the notebook experience as well, pretty much nothing from our extension works if there is no Julia installed...

I think in terms of message probably best to just point users to julialang.org, right?

This change would probably be best rolled out once the Julia extension official supports the notebook in the stable version

Agreed!

I think what I'll work on next is to not open a terminal per kernel, but instead add UI in the Julia Workspace for running kernels. I think that could work well, or did you plan to have shared UI in the Jupyter extension somewhere that shows all running kernels with options to stop and restart?

@DonJayamanne
Copy link
Collaborator

somewhere that shows all running kernels with options to stop and restart?

Can we hold off on that for a while. I'd like to check with the team

@davidanthoff
Copy link
Member Author

@DonJayamanne I just merged #2228 into the notebook branch. That PR integrates the notebook kernels into our workspace view, and that would be a pretty natural way now to also surface a stop and restart button.

@davidanthoff davidanthoff changed the title Add initial notebook provider implementation Add notebook controller implementation Jun 21, 2021
@davidanthoff
Copy link
Member Author

@DonJayamanne Just a quick FYI, this is how the UI now looks for exploring variables and controlling kernels:

image

So there is now a top level node for the Julia REPL and each notebook kernel in our Workspace view, and then one can drill down to see the variables in each of these, and further drill down into arrays, dicts and structures etc.

There is also a button to restart and stop there for each kernel, which I think for now is good enough.

I'm going to fix some pretty basic bugs around restarts/stop and cleanup next, lifecycle for the various things is a bit of a mess right now :)

Have you started with the multi Julia version support? If not, I could also take a stab, I'm well into the code base at the moment and have some time.

@davidanthoff
Copy link
Member Author

@DonJayamanne I'm going to start the multi Julia version implementation now, fits in very nicely where I am right now :)

@davidanthoff
Copy link
Member Author

@DonJayamanne Alright, this is really coming together :) One question: what would happen if we were to merge this into master right now and started shipping it? Essentially nothing, right? Because the shipping Jupyter extension on VS Code Release has not yet switched to the native notebook UI? So would the situation essentially be that for users that use VS Code Insiders, they would get all the new stuff, but everyone else would just not even notice that we are shipping this stuff? I'm kind of tempted to merge this rather sooner than later, before this PR gets too large und unwieldy...

@DonJayamanne
Copy link
Collaborator

ne question: what would happen if we were to merge this into master right now and started shipping it? Essentially nothing, right? Because the shipping Jupyter extension on VS Code Release has not yet switched to the native notebook UI? So would the situation essentially be that for users that use VS Code Insiders, they would get all the new stuff, but everyone else would just not even notice that we are shipping this stuff? I'm kind of tempted to merge this rather sooner than later, before this PR gets too large und unwieldy...

Sorry, some how i missed this message.
I'd say go for it, that would be great.

o would the situation essentially be that for users that use VS Code Insiders, they would get all the new stuff

Yes

but everyone else would just not even notice that we are shipping this stuff

Yes, though some in Stable VS Code also get this new Notebook experience. We're rolling it out to a few users in stable slowly (controlled roll out).

I'm kind of tempted to merge this rather sooner than later, before this PR gets too large und unwieldy...

Agreed.

Again, as all of the API used is currently in stable VS Code there should be no problem in merging this today.

@davidanthoff davidanthoff marked this pull request as ready for review June 25, 2021 17:30
@davidanthoff davidanthoff modified the milestones: Backlog, Next Minor Jun 25, 2021
Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

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

LGTM. Probably best to get this in as soon as possible so we can fix some issues before JuliaCon :)

@davidanthoff davidanthoff merged commit f6fc99a into master Jun 29, 2021
@davidanthoff davidanthoff deleted the notebook branch June 29, 2021 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.