Closed mahendrapaipuri closed 11 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
e1c6d13
) 97.59% compared to head (e0e4128
) 97.59%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hiya folks
I'm slowly going back to business; I was able to give a manual test at commit 0691d4c3 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:
without any configuration
jupytext.toml
)at that point I decided I'd install a couple extra kernels for that purpose I picked
jupyter kernelspec list
Available kernels:
calysto_bash /Users/me/miniconda3/envs/jupytext/share/jupyter/kernels/calysto_bash
python3 /Users/me/miniconda3/envs/jupytext/share/jupyter/kernels/python3
javascript /Users/me/Library/Jupyter/kernels/javascript
in this context what I see :
another comment here: one could expect to see more entries in these two places Personally I have until now never used fancy formats like, I may be making this up, sh:percent or js:percent, primarily because of the absence of the current feature (typically my bash notebooks would be stored as md:myst for example) But I was a bit hoping to be able to do this sort of stuff, so I was a little confused here again this is the naive and candid perception; I acknowledge there may be millions of good reasons for doing it this way, just trying to reflect a new user's likely astonishment I think I understand this is the result of the limitations described in your post
at that point I went to messing with the configuration panel
auto:
entries in my case resulted in one entry each; same limitations again I presumeon a totally side note:
around one year ago I have decided to name all my text notebooks in something like *-nb.md
or *-nb.py
and this has helped me a lot because when you reach hundreds of files it helps to have a clue about which are notebooks and which are not
I suggest we take this chance, and as a hint to users, that we name the newly created notebooks in e.g. Untitled-nb.md
if that resonates with you I can create a separate Issue to describe that..
otherwise 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 @mahendrapaipuri for all the work involved in making this happen !
Thanks a lot @parmentelat for your detailed testing. Really helpful feedback.
I can see the launcher populated with one jupytext row that has 5 boxes for py:light, py:percent, markdown (not quite sure which variant though) MyST Markdown and R Markdown - a first comment here, I have no R kernel installed, I'm not sure I see the relevance of that last entry
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 R
? These are my assumptions. I guess @mwouts will have a better answer to it.
But I was a bit hoping to be able to do this sort of stuff, so I was a little confused here again this is the naive and candid perception; I acknowledge there may be millions of good reasons for doing it this way, just trying to reflect a new user's likely astonishment I think I understand this is the result of the limitations described in your post
Precisely. This is the limitations I was talking about. The thing is we should know the file format of the kernel (.sh
for calysto_bash and .js
for javascript) along with some metadata. This info is not readily available from kernel specs. We can try to guess and maybe use some info provided in JupyterLab's code-mirror package but there is no guarantee that it will work for arbitrary kernels. Maybe we can let users to configure this metadata as well? In that case users will need to configure the kernel's file extension and name of kernel and we can use this file metadata to pick up these arbritrary kernels and add text notebook entries.
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.
I suggest we take this chance, and as a hint to users, that we name the newly created notebooks in e.g. Untitled-nb.md if that resonates with you I can create a separate Issue to describe that..
What I would suggest is making it configurable via Settings. By default it will be Untitled
but you user can choose the name of it. @mwouts What do you think?
@parmentelat Could you please try the latest commit? I tried to add support for arbitrary kernels and in your test env, bash
and js
kernels should pop up in Jupytext category.
hi so moving to commit 4f705c8e
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 auto:
setting
What I would suggest is making it configurable via Settings. By default it will be Untitled but you user can choose the name of it. @mwouts What do you think?
I like the idea of it being configurable, but I'd still suggest the default to be Untitled-nb
:)
@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 js
kernels pop up. Could you please confirm it?
I do confirm and with 4c181e35 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
@parmentelat it makes sense that you want to use Untitled-nb.md
for your notebook name (but I would prefer to keep Untitled.md
as the default). In the Python library, the format descriptor can use a suffix. Take this example:
$ jupytext Untitled.ipynb --to '_nb.md'
[jupytext] Reading Untitled.ipynb in format ipynb
[jupytext] Writing Untitled_nb.md
Do you think you could use -nb.md:myst
as your format descriptor in the config? (Note at the CLI we can't seem to start the format with a minus, so I have used underscore).
The .md
format is Jupytext's first markdown format: https://jupytext.readthedocs.io/en/latest/formats-markdown.html#jupytext-markdown. MyST Markdown is much more advanced.
R Markdown is a notebook format developed by RStudio that has existed for a very long time. It is probably mostly used for R notebooks, but it can actually contain any language. A few years ago a successor to that format was introduced, Quarto (.qmd
).
The text notebook created with either the menu or the launcher should open (when it is created) with the notebook editor. @parmentelat , I see that you have created https://github.com/mwouts/jupytext/issues/1164 , does this occur when you re-open the document or also when it is created?
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
• Do you think we should have the same multiple languages in the Menu as in the launcher?
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 in any case I stress again that my above answer is about an ideal world :-)
@mwouts Cheers for the inputs!!
Do you think we should have the same multiple languages in the Menu as in the launcher?
I thought that we cannot add entries to Main Menu programatically as we are setting them at build time in plugin.json
file. But when I installed R kernel in my local dev env, a new entry in Main Menu File>New>New R File
popped up. So, I guess there is a way to add them on fly based on available kernels. I will look into it and see what we can do. If we manage to achieve, that would be a perfect scenario.
it makes sense that you want to use Untitled-nb.md for your notebook name (but I would prefer to keep Untitled.md as the default). In the Python library, the format descriptor can use a suffix.
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?
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 thought that we cannot add entries to Main Menu programatically as we are setting them at build time in plugin.json file. But when I installed R kernel in my local dev env, a new entry in Main Menu File>New>New R File popped up. So, I guess there is a way to add them on fly based on available kernels. I will look into it and see what we can do. If we manage to achieve, that would be a perfect scenario.
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!
@parmentelat Cheers for the help!! I think I figured it out. I will update this PR. Let's not duplicate the work.
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 besides it would remove the need for reloading the page after settings changes
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!
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
Jupytext Notebook
factory so when user double clicks a text notebook of any format, it opens as notebook.Here is a screenshot of new UI
@mwouts @parmentelat Could you please test them out? Cheers!
Hiya there
a quick testing with commit f015227b: mostly works fine !
congrats for displaying the original kernel icons 👍 same goes for the kernel filetypes thing as well 👍 👍
a few comments in no particular order, giving my perception of priorities
medium: I find the contents of mainmenu/new notebook submenu to be a little too much I think I would prefer to find there the same entries as the (configurable) ones in the launcher, given that the whole family of cpmmands are available through the palette; maybe yet another option in the settings ? disclaimer: I still have some stakes in legacy environments that are totally notebook-minded, for which I have done quite some work to move to nb7; the thing is in these contexts there is no launcher thing, so mainmenu is the only visible way to create text notebooks; in this context even with a single Python kernel, we'd end up with 8 entries here, and that is a lot to choose from for beginners...
medium: as much as I love your new "double-cilcking just works" thing, I wonder if this should not also be a little configurable; this double-clicking business has been a pain for a loooong time already, and on occasions @mwouts has had reservations on a too open policy in this respect, so let's hear him on that specific; to be clear though, as far as I am concerned, your current implementation in this respect is just perfect
minor: the launcher contents does not quite comply with the order in the settings
I'm not sure I tested this before (I think not), but I found that in order in which I get the launcher entries does not quite follow the specs; looks like the auto entries
are treated a bit specially; I mean with this setting
I get
while I was expecting, I guess: MyST, the 3 auto percents, the rmd
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 ?
very minor:
imho the settings key should read formats
plural
there are a few places in the code where the singular makes things in a little awkward
we now have this
cat ~/.jupyter/lab/user-settings/jupyterlab-jupytext/plugin.jupyterlab-settings
{
"category": "Jupytext",
"format": [ # <---- this one I mean
"md:myst",
"auto:percent",
"Rmd"
]
}
very minor: I would suggest that verbose messages like
Registering pairing command=jupytext:pair-nb-with-auto:light with rank=1
# or
Registering create new text notebook command=jupytext:create-new-text-notebook-py:light with rank=11
be issued with console.debug()
to avoid clobbering the console
thanks a million @mahendrapaipuri
one last thing I just came across in this test Include Metadata toggle in Jupytext mainmenu is always grayed out in my setup how can I turn it back alive (i.e. not grayed out) so I can try it out ?
medium: I find the contents of mainmenu/new notebook submenu to be a little too much I think I would prefer to find there the same entries as the (configurable) ones in the launcher, given that the whole family of cpmmands are available through the palette; maybe yet another option in the settings ?
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.
medium: as much as I love your new "double-cilcking just works" thing, I wonder if this should not also be a little configurable;
I need to test it myself. But I think it is opening text notebooks as notebooks by default without needing any config on defaultViewers
, right? @mwouts Could you please detail the exact behaviour that you would like to have?
minor: the launcher contents does not quite comply with the order in the settings
Yes, the current implementation does not take the order in settings into the account. I will try to fix it.
very minor: I would suggest that verbose messages like
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.
hiya lads :)
thank you @mwouts for sharing your thoughts
Basically I think that we could indeed display fewer formats by default (just 'percent', multiplied by the number of languages, and 'MyST markdown').
we seem to have a consensus on keeping this menu not overpopulated ;) plus, as far as I understand from your comments in the code, you would agree to have the same contents in the Jupytext → New submenu as in the launcher, right ?
Also I think that we can make the configuration a bit more user-friendly by using check boxes rather than format names.
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
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?
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 Untitled
as the default, I'd say we're better off forgetting about making this name configurable; let's keep it simple, plus we can still add that later on if needed, but I don't think it will
Also, I am not sure why but on Binder (and actually also locally) I get an empty 'New Text Notebook' sub menu.
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 for the record, before @mahendrapaipuri took over I had been playing with a POC code that
Also, I am not sure why but on Binder (and actually also locally) I get an empty 'New Text Notebook' sub menu.
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
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.
we seem to have a consensus on keeping this menu not overpopulated ;) plus, as far as I understand from your comments in the code, you would agree to have the same contents in the Jupytext → New submenu as in the launcher, right ?
This has been modified. Now both Main Menu and Launcher will show same items.
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.
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.
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.
That is a very good idea and test. It is a bug and I fixed it. It should work now (at least I hope)
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.
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.
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.
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.
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!!
@mwouts @parmentelat Please test the new patch and see if we managed to address all issues.
here's my very few last comments; it's all minor and cosmetic, and none is a showstopper :)
minor: settings page
List of Jupytext Text Notebook formats that will be added to launcher please add "and to New Text Notebook submenu of main Jupytext menu" as well as will be added to launcher and menu
minor: in the settings page again, I think I liked someone's suggestion to explicitly add the (all languages) suffix i.e. phrase the 4 first options like e.g. Text Notebook as Script in Percent Format (all languages) or maybe just Script in Percent Format (all languages) instead of Text Notebook as Script in Percent Format PS: I've seen the text that spells it out, but like 20 minutes of testing later; people are lazy ;-)
very minor: it it possible to make this sentence a bit more conspicuous:
Refresh the current browser tab for the changes to take effect. i.e. in bold and on a line of its own ?
very minor: palette, I noticed an undesired entry labelled New undefined !?!
You guys been very patiently testing these changes. Cheers for that. I think we are getting closer.
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