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

opening jupytext notebook with a base_url is failing #6942

Closed
parmentelat opened this issue Jun 22, 2023 · 13 comments · Fixed by #6961
Closed

opening jupytext notebook with a base_url is failing #6942

parmentelat opened this issue Jun 22, 2023 · 13 comments · Fixed by #6961
Labels
Milestone

Comments

@parmentelat
Copy link
Contributor

parmentelat commented Jun 22, 2023

Description

possibly related to #6914

getting a 404 error when using the Open withNotebook menu on a jupytext notebook

nb7-text-base-url

this capture was made when running

jupyter lab --NotebookApp.base_url=/jlab4-nb7-text/

the 2 first first attempts work fine:

  • on a ipynb, Open withNotebook
  • on a jupytext, Open withJupytext Notebook

the video may not be super clear but in both these cases the new tab's URL contains the base_url

BUT

  • on a jupytext, Open withNotebook ends up on a 404 because the base_url thingy was not inserted in the URL

Reproduce

see above; this is with

pip freeze | egrep 'jupyte|notebook'
jupyter-events==0.6.3
jupyter-lsp==2.2.0
jupyter_client==8.2.0
jupyter_core==5.3.1
jupyter_server==2.6.0
jupyter_server_terminals==0.4.4
jupyterlab==4.0.2
jupyterlab-pygments==0.2.2
jupyterlab-widgets==3.0.7
jupyterlab_jupytext @ git+https://github.com/mwouts/jupyterlab-jupytext.git@d451f5c6fddc958bd3b7082720a7627aa4681fbd
jupyterlab_server==2.23.0
jupytext==1.14.6
notebook==7.0.0rc1
notebook_shim==0.2.3

Context

Command Line Output
[W 2023-06-22 12:55:39.037 ServerApp] 404 GET /edit/tests/sample-jupytext.md?factory=Notebook (cddb81bae87041cc81171cb9a9812c73@::1) 1.64ms referer=http://localhost:8891/jlab4-nb7-text/tree/tests
Browser Output

image

@parmentelat parmentelat added bug status:Needs Triage Applied to issues that need triage labels Jun 22, 2023
@jtpio jtpio added this to the 7.0 milestone Jun 22, 2023
@jtpio
Copy link
Member

jtpio commented Jun 22, 2023

Thanks @parmentelat for reporting.

Another odd thing when looking at the screencast above is that when selecting Open With > Notebook, then it should redirect to <base-url>/notebooks and not <base-url>/edit.

Maybe it's an issue with this handler:

notebook/notebook/app.py

Lines 332 to 337 in 8efd94a

self.handlers.append(
(
rf"/{self.file_url_prefix}/((?!.*\.ipynb($|\?)).*)",
web.RedirectHandler,
{"url": "/edit/{0}"},
)

@parmentelat
Copy link
Contributor Author

with the change from https://github.com/jupyter/notebook/actions/runs/5345232824?pr=6943, what I see for the URLs is this

# Open with → Notebook
http://localhost:8889/jlab4-nb7rc2-text/edit/tests/sample-jupytext.md?factory=Notebook
# Open with → Jupytext Notebook, I get simply this
http://localhost:8889/jlab4-nb7rc2-text/edit/tests/sample-jupytext.md

as you can see in both cases the URL contains /edit/, while the plain ipynb notebook contains /notebooks/ like so

http://localhost:8889/jlab4-nb7rc2-text/notebooks/tests/sample-notebook.ipynb

@jtpio
Copy link
Member

jtpio commented Jun 22, 2023

This is likely because of the folloiwing regex if the file extension is .md.

rf"/{self.file_url_prefix}/((?!.*\.ipynb($|\?)).*)",

@parmentelat
Copy link
Contributor Author

parmentelat commented Jun 22, 2023

I am also seeing another couple differences between the notebook views for a ipynb and a jupytext :

  • the ipynb view has 2 additional menu entries Run and Kernel
  • and, much more cosmetic, there's an additional vertical space on top of the jupytext view

Edit: it appears the second problem is an artefact of the scrollbar, so please let's forget about that...

see the screenshot below

I guess it might be related to the /notebooks/ vs /edit/ diff in the URL ?

image

Edit
I just tried a py:percent jupytext notebook and it unsurprisingly behaves like the md:myst

@jtpio
Copy link
Member

jtpio commented Jun 22, 2023

I guess it might be related to the /notebooks/ vs /edit/ diff in the URL ?

Yes. The /notebooks page has some CSS overrides so the notebook looks like the classic notebook. And the /edit page disables some menu items:

case 'edit':
menu.kernelMenu.dispose();
menu.runMenu.dispose();
break;

Maybe the proper fix would be to make sure the Jupytext notebooks are open with /notebooks, at least when choosing the Notebook factory.

Not sure if this redirect to /edit would still be needed then. I think it was added in RetroLab a while ago, would need to find the related PR for more context.

@parmentelat
Copy link
Contributor Author

Maybe the proper fix would be to make sure the Jupytext notebooks are open with /notebooks, at least when choosing the Notebook factory.

totally agreed :)

@jtpio
Copy link
Member

jtpio commented Jun 22, 2023

Not sure if this redirect to /edit would still be needed then. I think it was added in RetroLab a while ago, would need to find the related PR for more context.

It was added in jupyterlab/retrolab#98

@parmentelat
Copy link
Contributor Author

parmentelat commented Jun 22, 2023

I gave that a quick try within the same conda env
(using jupyter notebook file instead because of course Jupyter command 'jupyter-classic' not found.)

jupyter notebook sample-notebook.ipynb

yields this

image

and same with the other sample notebooks, so that feature seems to have grown broken in the meanwhile, has it not ?

@jtpio
Copy link
Member

jtpio commented Jun 23, 2023

and same with the other sample notebooks, so that feature seems to have grown broken in the meanwhile, has it not ?

Just tried locally and it seems to be working:

  • With a notebook it opens under /notebooks
  • With another file it opens under /edit

@jtpio
Copy link
Member

jtpio commented Jun 23, 2023

One idea could be to modify the regex so the redirect handler does not apply when the URL contains ?factory=Notebook:

rf"/{self.file_url_prefix}/((?!.*\.ipynb($|\?)).*)",

@RRosio
Copy link
Collaborator

RRosio commented Jul 5, 2023

Just wanted to share that I've tried Notebook 7.0.0rc2 in which providing a base_url:

  • Opening a Jupytext Notebook with Jupytext Notebook directs to {base_url}edit/{notebook_name}?factory=Jupytext%20Notebook
  • Opening a Jupytext Notebook with Notebook directs to {base_url}notebooks/{notebook_name}?factory=Notebook

@parmentelat, did Notebook 7.0.0rc2 address this problem or did I misunderstand this issue?

@parmentelat
Copy link
Contributor Author

parmentelat commented Jul 6, 2023

so, 7.0.0rc2 does address the initial concern, in that I no longer get a 404 and the notebook does open

however as you noticed, the /edit/ path is new as compared to the classic notebook experience
I believe @jtpio had tried to get rid of this discrepancy at some point
imho it would be great if we could achieve this, as the presence of 2 different paths has potential harmful side effects:

  • it becomes possible to have 2 tabs opened on the same notebook, but that don't know they share the same file; I'm not exactly sure about that, I am no expert jupyterlab user (yet), but it feels wrong
  • also external toolings may be confused if they rely on the fact that the current URL contains /notebooks/ in order to parse for the notebook location or similar

so to summarize, I can live with what 7.0.0rc2 gives us, but I quite agree that there remains possible improvements

@RRosio
Copy link
Collaborator

RRosio commented Jul 6, 2023

Thank you for the detailed clarification @parmentelat!
I've opened #6961 in hopes that it will address your points!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants