search

damianavila / RISE

RISE: "Live" Reveal.js Jupyter/IPython Slideshow Extension
Other
3.66k stars 414 forks source link

New approach for JupyterLab 3 #605

Closed fcollonval closed 1 year ago

fcollonval commented 2 years ago

This is a reboot of #381 - in order to move forward on https://github.com/jupyter/notebook/issues/6220

The approach taken differs from #381 as it creates a stand-alone app (similarly to retrolab - JLab remix) in order to:

What it does:

What it partly do:

What needs to be done:

Help will be really appreciate to cover the above points - don't hesitate to open PRs against this PR branch.

rise_demo_20211120

fcollonval commented 2 years ago

Binder Link

parmentelat commented 2 years ago

sounds cool, but the binder looks broken, or what am I missing ?

404-not-found

fcollonval commented 2 years ago

Unfortunately it does not work on Binder... needs to set the redirection.

parmentelat commented 2 years ago

no big deal if binder won't work this is going to give us an opportunityto run the code from source how should we go about doing that ?

damianavila commented 2 years ago

This is AWESOME, @fcollonval!! I will try to digest the content ASAP and provide feedback!! At first, the standalone app is an interesting compromise to get people running on JLab ASAP. Super excited about this, thanks again for the contribution!

fcollonval commented 2 years ago

sounds cool, but the binder looks broken, or what am I missing ?

@parmentelat I fixed Binder if you want to try.

parmentelat commented 2 years ago

@fcollonval this looks just great, it is several orders of magnitude better than anything we've been trying to do as a jlab extension; many thanks !

I have to honestly admit that I have not been able to cope with the jlab extension dev paradigm(s); so how do I setup a dev environment to proceed on your track ?

I tried this just now:

git switch jlab3
git rev-parse HEAD
→ 42a9d9aa501894ee2d02782c08d364c287d9a5f9
conda create -y -n rise python=3.9
conda activate rise
pip install jupyterlab
pip install .
jupyter lab --version
→ 3.2.3
jupyter lab

but unsurprisingly it does not seem to work, I can't see the RISE button and am getting this screenshot below

it feels real dumb, please enlighten me :)

image

parmentelat commented 2 years ago

on a totally different level, one thing that I had at some point considered - before realizing that developing a jlab extension was way beyond my skills - was to have the regular notebook and the rise view in 2 separate tabs this way I thought ideally we would have gotten the notebook view and the rise view side-by-side, so as to, for example, have a live preview of the rendered slides while tweaking the notebook

this was imho the most important breakthrough that porting to a jlab extension could bring us

I'm not quite sure this is doable at all though, so if this rings any bell wrt this new angle, please elaborate on the feasibility of that idea :)

fcollonval commented 2 years ago

how do I setup a dev environment to proceed on your track ?

Here are the commands to execute (I updated the dev note):

    yarn install
    yarn run build
    pip install -e .
    jupyter server extension enable rise
    jupyter serverextension enable rise
    jupyter labextension develop --overwrite .
    jupyter-nbextension install rise --py --sys-prefix --symlink
    jupyter-nbextension enable rise --py --sys-prefix

the notebook view and the rise view side-by-side

This should be achievable with the new approach by rendering the presentation in a IFrame like the voila preview

Edit: correct command for dev mode

parmentelat commented 2 years ago

Hi thanks for the dev recipe I've just given that a try and am getting mixed results I do see the new extension button now, but that gives me a 404 not found - just like the first gif above in fact not sure how to troubleshoot that issue though

fcollonval commented 2 years ago

You may need to install the server extension for the old notebook based server with:

jupyter server extension enable rise
parmentelat commented 2 years ago

yes, this last command does the trick !

parmentelat commented 2 years ago

have tested on jupytext-encoded notebooks; does not work for me; I do see the rise button; pressing it opens a new browser tab labeled 'Rise' but it remains empty

parmentelat commented 2 years ago

need to add to the things to test: ipywidgets and interact()

fcollonval commented 2 years ago

@parmentelat following our discussion I pushed a description of the folder structure with pointers where is what in the dev notes

cc @damianavila you may be interested by this too :wink:

asaboor-gh commented 2 years ago

I am waiting for RISE to work in Jupyterlab as expected. Alongside it, you can include IPySlides in your toolbox to have full control over content display in a slideshow

damianavila commented 2 years ago

@fcollonval, I plan to dive into this PR soon (next few days) and I want to make that process as smooth and efficient as possible. This is currently a big PR with some JupyterLab extension boilerplate, some RISE boilerplate, and a lot of awesome work from you building on top of that. What would be your suggestion to make a reasonable and efficient review? Should I read JupyterLab pieces, examples, or any other pieces before attempting to provide feedback on this one? Otherwise, I can jump as is and probably do some really silly questions... but I guess that will be OK-ish, right 😜 ?

fcollonval commented 2 years ago

What would be your suggestion to make a reasonable and efficient review? Should I read JupyterLab pieces, examples, or any other pieces before attempting to provide feedback on this one?

I updated the doc/dev/develop.md to bring some information about the proposed repo structure. The extension for the classical notebook and the patching of reveal.js have not been modified in term of content. They only have been moved to support multiple packages paradigm (they lie respectively in packages/classic and packages/rise-reveal).

The interesting pieces are:

The machinery to create a standalone application is in the app folder - probably the more obscure part (unfortunately there is not much documentation on that front - maybe looking at https://github.com/jtpio/jupyterlab-app-template could help). But I will suggest not spending too much time on that one.

To get a proper stand-alone application, a application object and in particular a shell for the widget needs to be defined - this is done in packages/application/src/app/index.ts. This is a tiny class fulfilling the interface required by any Jupyter frontend (using JupyterLab like packaging mechanism).

probably do some really silly questions... but I guess that will be OK-ish, right ?

Please don't hesitate to do so.

parmentelat commented 2 years ago

Hiya lads !

I've been messing with Frederic's new stuff, and have been able to improve markupSlides to take into account the 5 types of slide cells, mainly by porting the old code

thanks again to Frederic for his help with this entirely new (to me at least) dev mode :) the yarn run watch thing is particularly convenient, I have to say !

otoh I have not been able to create a PR of my jlab3 branch against frederic's

so until I figure how to do this (or we come up with some simpler process), I wanted to share with you my code here https://github.com/parmentelat/RISE/tree/ft/jlab3

I'd primarily like to get some feedback on any poor practice that I might have used, given that it's my first go at typescript ever :)

next step for me would be to implement auto-selection

fcollonval commented 2 years ago

@parmentelat thanks a lot - I opened the PR against this branch with your modifications: https://github.com/fcollonval/RISE/pull/1

I'll comment on it.

parmentelat commented 2 years ago

question for you @fcollonval:

when creating the RISE tab in the first place, I'd like to restore the active cell from the one open in the 'regular' jlab view (the one where I press the slideshow button in the first place)

however when I inspect the notebook instance that I get in markupSlides, its activeCellIndex is always 0 it's obviously a separate object that corresponds to then one in the slideshow view, and the info about active/selected cells seems to have gone under the bus I'm not really concerned about the selected cells, but the active one is something we are used to rely upon so the slideshow can start from where we left off

can you please help me get that right ?

fcollonval commented 2 years ago

can you please help me get that right ?

I would suggest passing the index through query argument. So this means

  1. Changing packages/lab/src/index.ts#getRiseUrl to add the index in the URL
    You can get the URL from current.content.activeCellIndex in https://github.com/fcollonval/RISE/blob/acae3fd1b001b0eaacb3d023424e7a9cdae17b27/packages/lab/src/index.ts#L142 and the factory needs to be changed (that is not gonna be easy because the factory is tied to a file not a view).
  2. In https://github.com/fcollonval/RISE/blob/acae3fd1b001b0eaacb3d023424e7a9cdae17b27/packages/application/src/plugins/index.ts#L25, you need to read the value of the active index from the query argument and set it to the notebook before apply markupSlides
parmentelat commented 2 years ago

mmh, I get some of this information, but not all of it :) so I have started with the code in plugins/index.ts where I feel most comfortable with a hard-wired activeCellIndex at this point I have another issue, which is to wait for reveal to have loaded before I can use it for anything here again I have hard-wired something just to be able to move forward

this is pushed in my branch if you want to take a look

thanks !

fcollonval commented 2 years ago

Big update today

I import all the code from the classical plugin and make works most of it:

damianavila commented 2 years ago

Wonderful news, @fcollonval!! Hopefully, I can give you useful feedback pretty soon!

damianavila commented 2 years ago

@fcollonval, I tried to run this branch and I found some errors at the very beginning of my journey 😉 I was not able to successfully yarn install nor yarn run build I debugged for some time and I have pushed a PR against your branch with some changes: https://github.com/fcollonval/RISE/pull/2 Let me know if those make sense to you!

Btw, I played for some time with the CSS and made some progress... but I think I would need to take another approach starting from a clean/scratch CSS because the current styles on the base.css (at the application level) are not actually working since they were tweaks for the legacy notebook...

Finally, some things I quickly noticed:

Btw, in the following days, I hope I can provide you with feedback on the jupyterlab-specific and RISE lab-specific code!

fcollonval commented 2 years ago

Thanks a lot for reviewing and testing

I tried to run this branch and I found some errors at the very beginning of my journey 😉 I was not able to successfully yarn install nor yarn run build I debugged for some time and I have pushed a PR against your branch with some changes: fcollonval#2 Let me know if those make sense to you!

Thanks for that

Btw, I played for some time with the CSS and made some progress... but I think I would need to take another approach starting from a clean/scratch CSS because the current styles on the base.css (at the application level) are not actually working since they were tweaks for the legacy notebook...

This sounds like a better approach - especially because the styles were looking better before I imported the one from main.css of the classical notebook.

* I was not able to make the speaker notes work
* Neither the W option to see from far above

Sounds like the shortcut selector needs to be tuned because it works if you select a cell for example (see screencast). But indeed not by default - something about the focused element.

demo_shortcuts

* I was expecting to see more RISE-specific commands in the cmd palette, not sure why I can not see them

You need to type text appearing in the command description (unfortunately the category is not shown...) - I should definitely open a PR for that. And we can definitely increase the list at https://github.com/damianavila/RISE/blob/c35a62022dd0d11e500b21acf8904e2d3079dc5a/packages/application/src/plugins/rise.ts#L96

Btw, in the following days, I hope I can provide you with feedback on the jupyterlab-specific and RISE lab-specific code!

🎉

parmentelat commented 2 years ago

Hi

I gave this a try as well; I started my repo from scratch and apparently the fixes are working

my comments at this point:

what puzzles me is, it feels like I have 3 different notebooks at work here, and changing one does not reflect on the others

image

and if that's the model, imho we are getting into very dangerous area: I fear it might become really hard to make sure one does not make concurrent changes from 2 different angles, resulting in data being lost

sorry for mixing many things together in this long post; and thanks for reading me thus far ;)

fcollonval commented 2 years ago

Thanks for testing @parmentelat

I will try and investigate that matter a little further, and

:heart:

  • I have some confusion with the workflow; so IIUC: . I start with the notebook in a regular jlab tab . when clicking the rise icon in a 'regular' notebook, it opens something that for lack of a better idea I'll call a jlab-rise-tab; . and if I click the rise icon from that embedded-rise tab, it opens a full-fledged browser-rise-tab

In the current approach the slide show view is totally independent of JupyterLab. Hence there is no state synchronization between various views. This is the reason you are seeing those warnings.

There may be a way to not use an independent window (aka not an iframe) to allow synchronization of the view. The constrain will be that only one presentation can be displayed at a time (this is probably an acceptable restriction). This will requires a bit more work than the current approach. But it may worthy.

Regarding the webbrowser tab version, I kept it for now to allow some debugging (to avoid dealing with an iframe within JupyterLab). But I agree, it should probably disappear when the user as JupyterLab as entrypoint.

besides, I'd like to mention that reveal has a 'full screen' feature

Yes that is exactly what I would like to fix

damianavila commented 2 years ago

Thanks for the feedback on my comment @fcollonval, I will follow up soon after checking/testing what you have indicated.

Regarding what @parmentelat suggested about the workflow...

In the current approach the slide show view is totally independent of JupyterLab. Hence there is no state synchronization between various views. This is the reason you are seeing those warnings.

I was expecting that answer 😉

There may be a way to not use an independent window (aka not an iframe) to allow synchronization of the view. The constrain will be that only one presentation can be displayed at a time (this is probably an acceptable restriction).

My original prototype PR #381 was built on that idea 😉 but then you have the problem of the reveal.js colliding with JupyterLab as I mentioned on that PR (https://github.com/damianavila/RISE/pull/381#issuecomment-722029634). In fact, I actually like the current iframed approach because it isolates stuff preventing future clashes with reveal.js. In the longer term, I am not that worried about the current "overwriting issue" because when the notebook model is finally persisted server-side (and I think that will eventually land, right?) we should be OK and it would be a matter of just refreshing the views to get the latest state on each of the views we have... In the short term, I think any sync on the client-side is a whole new problem/piece of work so we might be OK with just having a way to signal which view should be saved and forbid any savings on other views, maybe, WDYT? I think that should be enough to continue building the current idea/implemetation...

This will requires a bit more work than the current approach. But it may worthy.

@fcollonval, do you have an idea about what would be the amount of work involved (modulo we resolve the reveal.js clashing issue, I think the multi presentation support in reveal.js 4 could help here, but still we will have reveal.js intrusion on the JupyterLab application 🤔 )

Cheers,

fcollonval commented 2 years ago

In the longer term, I am not that worried about the current "overwriting issue" because when the notebook model is finally persisted server-side (and I think that will eventually land, right?) we should be OK and it would be a matter of just refreshing the views to get the latest state on each of the views we have...

Yes this is a WIP with ETA beginning of next year. And indeed that will avoid that issue.

In the short term, I think any sync on the client-side is a whole new problem/piece of work so we might be OK with just having a way to signal which view should be saved and forbid any savings on other views, maybe, WDYT? I think that should be enough to continue building the current idea/implemetation...

I don't think the current API will allow us to do something like that. And I don't think we want to block edition/execution within RISE...

This will requires a bit more work than the current approach. But it may worthy. do you have an idea about what would be the amount of work involved

The idea is still a bit different than #381. It will require to define our own way to produce the DOM from a notebook model (instead of modifying the one lay down by the current notebook editor). It should be possible to cut a PoC in 2-3 days. Then much testing will be needed - especially to deal with cell addition, deletion, displacement. And the other con is that it will naturally diverge from the reference notebook editor (the rate of divergence will depend on the ability to directly inherit from the default editor or not).

damianavila commented 2 years ago

Yes this is a WIP with ETA beginning of next year. And indeed that will avoid that issue.

That is good to know!

And I don't think we want to block edition/execution within RISE...

I am not saying we should block edition/execution within RISE unless the user "marks" the RISE view to be "non-savable". What I am proposing is somehow detecting we are dealing with the same notebook and have some button? to pin the view we want to save and then the other ones just being editable/runnable but not able to save themselves...

the rate of divergence will depend on the ability to directly inherit from the default editor or not).

If the rate of divergence is small, then it could be "acceptable" until the server-side persistence of the notebook model arrives... if the rate is big, I would be worried about the situation we are getting into and how to sanely come back to the state we currently are that should work nicely in the future server-side model... some additional thoughts? 😉

fcollonval commented 2 years ago

I raised the point at the JupyterLab Team Meeting. And rightly Nick proposed to suggest turning on collaboration to avoid the infamous conflict pop-up (RTC is happening in the front-end with synchronization between multiple clients).

So I propose to detect if that flag is turn on or not (it needs to be activated at the command line or in the server configuration). If it is not, we could redirect people to some documentation to do so.

WDYT?

damianavila commented 2 years ago

I raised the point at the JupyterLab Team Meeting. And rightly Nick proposed to suggest turning on collaboration to avoid the infamous conflict pop-up (RTC is happening in the front-end with synchronization between multiple clients).

Ja... so we already have the front-end sync between multiple clients I asked before...

In the short term, I think any sync on the client-side is a whole new problem/piece

How could I forget about RTC 🤦 ??? At least I was right about the "whole new problem piece" 😜

So I propose to detect if that flag is turn on or not (it needs to be activated at the command line or in the server configuration). If it is not, we could redirect people to some documentation to do so. WDYT?

I am totally OK with that!

parmentelat commented 2 years ago

er, guys, look, I'm not sure when you've switched to mandarin chinese, but you did lose me here :)

damianavila commented 2 years ago

@parmentelat, RTC means real-time collaboration and there has been a lot of work in the last few years to bring RTC capabilities into JupyterLab. You can read more about this feature here: https://jupyterlab.readthedocs.io/en/stable/user/rtc.html

With RTC enabled (which is not enabled by default), the notebook content would be synchronized on the client-side for any of the notebook-based "views" we may have, which means the "notebook view" and the "RISE views" (either inside Lab or outside Lab) would no longer elicit the "File Changed" warning.

@fcollonval can surely explain things better than me if you need more details (Frédéric, feel free to correct if I am writing crazy stuff 😉 ).

parmentelat commented 2 years ago

oh ok, thanks for the clarification
I'd heard plenty about new 'collaborative edition' in the past months, so I take it this is related :) so that brings other questions along:

does all this make any sense ?

asaboor-gh commented 2 years ago

Looks like RISE does not support ipywidgets in Jupyterlab image

firasm commented 2 years ago

SO excited to see so much progress on this! Thank you to all involved! Happy to help with user-testing when we're at that stage!

fcollonval commented 2 years ago

Looks like RISE does not support ipywidgets in Jupyterlab

Could you execute the cell in the RISE slideshow? It does work for me. I suspect you did not execute the cell in the slideshow, so the output is not trusted and in that case the rich output is not used.

fcollonval commented 2 years ago

SO excited to see so much progress on this! Thank you to all involved! Happy to help with user-testing when we're at that stage!

Thanks and please don't hesitate to test it online: Binder Link

damianavila commented 2 years ago

@fcollonval, I did not forget about this one... just EOY break is already here and it is difficult to find time to review and develop on top of this one, you know...

Please be patient with me, I truly appreciate all the work you have done here and I hope I can give you more feedback soon.

Thanks again!!

parmentelat commented 2 years ago

hiya lads

I have tried to address the styles thing in this other PR here https://github.com/fcollonval/RISE/pull/3

I did follow the recommendation to file that PR against @fcollonval's repo, but having not heard from that channel I am wondering if that was the right thing to do ?

fcollonval commented 2 years ago

Sorry @parmentelat I was not aware of notifications not being turn on for all activities on a forked. Thanks for the PR. I merged it.

henryiii commented 2 years ago

Would this work with JupyterLite? A completely in-browser interactive presentation would be really amazing! Also, would this be possible to ship as a pre-built extension for JupyterLab 3? Anyway, great work! Looking forward to this.

fcollonval commented 2 years ago

Would this work with JupyterLite? A completely in-browser interactive presentation would be really amazing!

Yes it will.

Also, would this be possible to ship as a pre-built extension for JupyterLab 3? Anyway, great work! Looking forward to this.

This PR is producing a pre-built extension for JLab 3.

strawpants commented 2 years ago

Wow thanks a lot for this. Really looking forward to using this in jupyterlab. One thing I noticed is that the rise.css file is not loaded in the jupyter lab version (both in binder and local installation). Can you confirm this? The custom css files named after the notebooks do get loaded.

fcollonval commented 2 years ago

One thing I noticed is that the rise.css file is not loaded in the jupyter lab version (both in binder and local installation). Can you confirm this? The custom css files named after the notebooks do get loaded.

The code is there to load any rise.css next to the notebook file - but there may be a bug.

damianavila commented 2 years ago

@fcollonval, FYI, I will be spending some cycles on this PR in the next few days. Sorry for the upcoming noise 😉 and thanks for your patience with me!

firasm commented 2 years ago

I'm sure everyone here is super busy but I wanted to just quickly add a note that JupyterBook now has support for jupyterlite via a sphinx extension so now we have a full juptyerlab session working in a browser without MyBinder or Thebe - meaning that if RISE works with JuptyerLab, I can take over the world :-)

Here's a working demo of what I mean about Jupyterlite: https://firasm.github.io/jb_jlite/intro.html from this relevant issue

(just imagine a little extension there to launch into "Rise mode" 🤩)