-
Notifications
You must be signed in to change notification settings - Fork 386
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
Language specific text notebook #1163
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1163 +/- ##
=======================================
Coverage 97.59% 97.59%
=======================================
Files 26 26
Lines 4357 4357
=======================================
Hits 4252 4252
Misses 105 105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hiya folks I'm slowly going back to business; I was able to give a manual test at commit 0691d4c and it feels really great ! before I describe my wanderings, I must say that I have not followed everything in the discussion, particularly the discussion on available kernels; so in this respect I wore my most naive and candid spectacles for the following what I've been able to test successfully: out of the boxwithout any configuration
multiple kernelsat that point I decided I'd install a couple extra kernels
in this context what I see :
another comment here: one could expect to see more entries in these two places configuringat that point I went to messing with the configuration panel
namingon a totally side note: around one year ago I have decided to name all my text notebooks in something like conclusionotherwise I did not run into any significant trouble during this test, everything ran smoothly regardless of my few comments, this is plain awesome ;-) |
Thanks a lot @parmentelat for your detailed testing. Really helpful feedback.
From my understanding, they all are different variants of Markdown. So, I guess R Markdown is some variant that R community uses? And it does not have anything to do with
Precisely. This is the limitations I was talking about. The thing is we should know the file format of the kernel ( And for the main menu, we cannot add programatically all the available kernels. That is why we are giving only Python text notebook options as Python kernel is guaranteed to exist.
What I would suggest is making it configurable via Settings. By default it will be |
@parmentelat Could you please try the latest commit? I tried to add support for arbitrary kernels and in your test env, |
hi still with the 3 same kernels as before (py + js + bash) For starters here's my settings screenshot, which gives me this so basically somehow only the js kernel triggered as part of the |
I like the idea of it being configurable, but I'd still suggest the default to be |
@parmentelat Cheers for quick testing. I see the reason why bash kernel does not pop up and there is indeed a more elegant way to implement what we are trying here. I pushed a new commit and that should fix it. In my local test env, both bash and |
I do confirm and with 4c181e3 I now get this I have been testing more with the newly created notebooks and so far I see nothing out of the ordinary |
Hi @mahendrapaipuri , @parmentelat , this is super exciting! Especially @mahendrapaipuri it's really great that you found a way to identify what languages are locally available, it will make it way easier to use. Quick questions
And a few notes
Do you think you could use
|
upon creation, the new document opens as a notebook all right; it's the second time around, when I try to re-open the notebook later on, that I face the problem described in #1164, and that is why I created a separate issue |
ideally i.e. regardless of any implementation consideration, I tend to think that the options offered in the launcher should match the ones in the menu; a user would customize that list to their most often used formats; there is still the palette to use any additional/exotic ones; and having 2 differents lists means 2 different configurations, which imho is overkill this being said, my understanding is there are severe implementation limitations, due to this antinomy between settings-defined vs program-defined ui elements; I'm not sure I have a 100% clear understanding of what is possible or not, so I might ask clarification questions on discourse about that |
@mwouts Cheers for the inputs!!
I thought that we cannot add entries to Main Menu programatically as we are setting them at build time in
In any case, we can still add an entry in the settings to configure this and default it to And regarding #1164, I think I know what is missing to be able to open them as Notebooks when a user double clicks them rather than text files. I will try to work on it. |
I've been working on an angle to achieve this, it seems to be doable indeed, maybe @mahendrapaipuri if you need to prioritize leave this to me for a while so I can report back |
@parmentelat Cheers for the help!! I think I figured it out. I will update this PR. Let's not duplicate the work. I think you raised an issue (which I couldnt find, sorry) about being able to download text notebooks with different formats. Something on this line. If you have time, please look into that issue. Cheers! |
fine; among the things I was about to propose was to react to settings changes, instead of (or in addition to) reacting to changes to kernel specs which, one may guess, are likely to happen on a very occasional basis
turns out the person requesting this feature is no longer eager to get it, so ... :) |
So, I managed to tweak the PR and add following functionalities
Here is a screenshot of new UI @mwouts @parmentelat Could you please test them out? Cheers! |
Hiya there a quick testing with commit f015227: mostly works fine ! congrats for displaying the original kernel icons 👍 a few comments in no particular order, giving my perception of priorities
thanks a million @mahendrapaipuri |
one last thing I just came across in this test |
I see your point and I would like to get @mwouts opinion as well before making further changes. Another setting option seems to me an overkill and confusing to the users.
I need to test it myself. But I think it is opening text notebooks as notebooks by default without needing any config on
Yes, the current implementation does not take the order in settings into the account. I will try to fix it.
I agree. I thought of doing it but forgot. Cheers for raising it. @mwouts I will wait for your feedback before I start any further changes. But globally we are getting closer to our objective. |
Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
- Based on available kernels we add launcher icons to create language specific text notebooks - Currently only python, julia and R kernels are explicitly supported - Metadata for jupytext pair and create commands include more details to improve UI Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
- We leverage the default languages map from codemirror to construct metadata of file type - We use a default icon for kernels other than Python, R, Julia Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
- Use constructed languages object in extension instead of instantiating a new one - Use findByName method to find language info. This method will account for alias as well - Use kernel's display_name for text notebook launcher icon Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
- We make a request to get kernel png and convert to base64 format to create svg - This svg is used to create kernel icon for each kernel Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
- We are able to pick up kernels at runtime and add corresponding jupytext entries in main menu - We register the kernel file types with jupytext notebook factory - We can open jupytext notebooks as notebooks when we double click Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
f015227
to
7de0744
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mahendrapaipuri , sorry it took me a long time to come back to this PR. Thank you for all the work you put into this! And thank you also @parmentelat for reviewing this with care!
I have added a few comments. Basically I think that we could indeed display fewer formats by default (just 'percent', multiplied by the number of languages, and 'MyST markdown'). Also I think that we can make the configuration a bit more user-friendly by using check boxes rather than format names.
In any case, we can still add an entry in the settings to configure this and default it to Untitled. So that we give freedom to users on how they would like to configure it. @mwouts Is it something you are keen to accept?
Yes, good idea, thanks!
minor: Jupytext → Pair Notebook → Custom Pairing results in an error message; it might be on purpose though, in which case maybe rephrase the message to not mention "Error" but "Warning" or something ?
Yes the idea for the custom pairing entry is just to display a ticked menu entry if the current notebook has a non-standard pairing. For now, if someone wants to set a custom pairing, they have to edit the jupytext.formats
metadata directly. This will be used by very few people so I don't think we need to address that in the extension.
Include Metadata toggle in Jupytext mainmenu is always grayed out in my setup
Oh thanks for checking that, yes it's better to make sure this one works too. Saving a notebook with metadata will include the yaml header in the script or md file.
Also, I am not sure why but on Binder (and actually also locally) I get an empty 'New Text Notebook' sub menu. You can see it here: https://mybinder.org/v2/gh/mahendrapaipuri/jupytext/lang_specific_text_nb . I don't have an obvious explanation, I wondered if that was caused by my latest rebase, by bash_kernel
that is installed there, or by jupyter_server==2.11
.
Finally I am curious about how you address the single click opening of non-ipynb notebooks, as notebooks. Do you use another mechanism than the default viewer configuration? Re your question about the default, I think that we should not change the default editor. Letting the user configure their preferred viewer seems a less invasive change.
Thank you again for the great work!!
jupyterlab/packages/jupyterlab-jupytext-extension/schema/plugin.json
Outdated
Show resolved
Hide resolved
jupyterlab/packages/jupyterlab-jupytext-extension/schema/plugin.json
Outdated
Show resolved
Hide resolved
jupyterlab/packages/jupyterlab-jupytext-extension/src/tokens.ts
Outdated
Show resolved
Hide resolved
jupyterlab/packages/jupyterlab-jupytext-extension/src/tokens.ts
Outdated
Show resolved
Hide resolved
...xt-menu.spec.ts-snapshots/opened-jupytext-menu-jupytext-new-text-notebook-jupytext-linux.png
Outdated
Show resolved
Hide resolved
jupyterlab/packages/jupyterlab-jupytext-extension/ui-tests/tests/jupytext-notebook.spec.ts
Outdated
Show resolved
Hide resolved
hiya lads :) thank you @mwouts for sharing your thoughts
we seem to have a consensus on keeping this menu not overpopulated ;)
imho the current scheme is good enough, and I'd rather keep it if only temporarily so that we can converge on something releasable soon enough; unless that suggestion turns out to simplify other issues (like ordering of entries); but I guess that's just me
I was the one to suggest this in the first place, but my major objective was to convey the suggestion to use specific namings for text notebooks; and since we're going to stick with
I've seen this behaviour once as well during at the beginning of my manual tests, but could not reproduce and thought no more of it, but now that you mention it too, there probably is something fishy going on there
same here, can you please elaborate on how this currently works ? I was kind of believing that the default viewer config was the only way to achieve this in summary I think we should focus on settling and releasing early, so as to get wider feedback about what we have right now, which is great - and thanks again @mahendrapaipuri for all the work |
* Use more convienent selectors for users to enable/disable formats Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
* We should not register the different kernel file exts with jupytext notebook type * We should let users to do this by overriding default viewers * This change will enable py files to open with default editors Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
* We cannot be sure that all kernels have same icon resources * Try one by one to get svg string * If none found, use generic kernel icon Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
* Read new settings file appropriately to get includeFormats array * Fix typos Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
* Test only enabled formats * Exclude metadata as snapshots should be changed for every version bump if we include metadata Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
This has been modified. Now both Main Menu and Launcher will show same items.
You need to either Pair a existing notebook or Create new text notebook for it to be enabled. I added it to UI tests to make sure it works.
That is a very good idea and test. It is a bug and I fixed it. It should work now (at least I hope)
Not sure about custom pairing. I havent changed any implementation logic of Pairing commands. @mwouts Could you please test it? If it is not working, I guess it should not block the current PR as the PR does not change code base of pairing commands. Let's create an issue if it does not work and work on it separetely.
I misunderstood that you would like to open text notebook text files as notebook by default. That is why I implemented that. Now, I reverted it back and by default when you double click text notebook file, it opens as a normal text file. Only when you change default viewer, it opens as notebook file. @mwouts @parmentelat Please test the new patch and see if we managed to address all issues. You guys been very patiently testing these changes. Cheers for that. I think we are getting closer. |
* Seems like url prefix is included in kernel icon url too. * We need to strip it off as baseurl will always have prefix included. Signed-off-by: mahendrapaipuri <mahendra.paipuri@gmail.com>
Thank you so much @mahendrapaipuri ! I've done some quick testing and everything seems to work as expected (many thanks for simplifying the options, I do find the new settings page much more user friendly this way!). Cheers also for fixing the binder. I will do some more testing tomorrow and try to provide a release candidate within a few days. |
@mwouts Please resolve the conversations if the response are clear for you so that we can follow them up easily. Cheers!! |
here's my very few last comments; it's all minor and cosmetic, and none is a showstopper :)
thank you @mahendrapaipuri, you are the one doing the heavy lifting on this one ;-) |
Awesome work! Thank you again @mahendrapaipuri |
Objective:
This is follow up work of #1154. The objective is to be able to create language specific Text notebooks from the JupyterLab launcher. In #1154 we are limited to creating only Python Text notebooks. This PR improves on that to be able to create language specific text notebooks from JupyterLab launcher. The following workflow is used to accomplish the goal:
Implementation:
auto:light
formats to found kernel types. For instance, if Python and R kernels are found,auto:light
will be replaced bypy:light
andR:light
and those launcher icons will be added to launcher.Example:
For instance, if we have both Python and R kernels installed, this is how launcher will look like
Settings:
If user use
auto:light
in Settings, it will add supported and installed launcher icons (Python and R in above screenshot) to create text notebooks. On the otherhand, if user is more explicit likepy:light
, even if R kernel is installed, the launcher icon will not be added.In any case, all the commands will be always available via Command Palette.
Main menu:
On the main menu, we cannot add these commands programatically, as we need to define the entries of the main menu in
plugin.json
beforehand.Limitations:
We are explicitly defining the supported kernels and thus, it will not work for any arbitrary kernel. This is due to that we need metadata of kernel like file extension, icon, name of kernel beforehand to be able to support arbitrary kernels. This is something that we can try to improve in future iterations. In any case, adding support for new kernels is as simple as adding entries in
tokens.ts
.Edit: This is now addressed in this commit. We try to support arbritrary kernels as long as a matching language metadata is defined in JupyterLab's codemirror package. But we cannot pick up their icons as they are not available by default and so we use default icons for kernels other than Python, R and Julia.
Other changes:
Icons are added for all commands in main menu.
Note:
Unfortunately, we cannot really control the icon of Jupytext category in the launcher. By the way it is implemented, it will always take the icon of first item in the category.
TODO:
@mwouts @parmentelat Can you please test it with different kernels installed in your local env. I have did very basic and quick tests.
Closes #1157 #1164