igvteam / igv-notebook

Module for embedding igv.js in an IPython notebook
MIT License
61 stars 14 forks source link

JupyterLab URLs #20

Closed bcjonescbt closed 1 year ago

bcjonescbt commented 1 year ago

I'm working on a JupyterLab server that is started OnDemand by my university, and I've been unable to load any local files.

It turns out that the URL of the session doesn't have "/lab/" (with trailing slash) in it, and that was breaking the URL encoding. A simple change (below) makes it work here, but I don't know what a general solution would be.

My URL looks like: https://xyz.edu/node/udc-ba25-32c1/35330/lab or sometimes https://xyz.edu/node/udc-ba25-32c1/35330/lab?

Here is the diff:

diff --git a/igv_notebook/js/messageHandler.js b/igv_notebook/js/messageHandler.js
index 8fbbe0d..a96d224 100644
--- a/igv_notebook/js/messageHandler.js
+++ b/igv_notebook/js/messageHandler.js
@@ -225,7 +225,7 @@
                 // URL is relative to notebook
                 return encodeURI(`${location.origin}${baseURL}files/${notebookDir}${url}`)
             }
-        } else if (pageURL.includes("/lab/")) {
+        } else if (pageURL.includes("/lab")) {
             // Jupyter Lab

             // Examples
@@ -235,7 +235,7 @@
             // https://hub.gke2.mybinder.org/user/igvteam-igv-notebook-5ivkyktt/lab/tree/examples/BamFiles.ipynb
             //    => https://hub.gke2.mybinder.org/user/igvteam-igv-notebook-5ivkyktt/files/examples/data/gstt1_sample.bam

-            const baseIndex = pageURL.indexOf("/lab/")
+            const baseIndex = pageURL.indexOf("/lab")
             const baseURL = pageURL.substring(0, baseIndex)

             if (url.startsWith("/")) {
jrobinso commented 1 year ago

The only general solution guaranteed to work, I think, is to use the full absolute URL to the file. However I will do some testing with your suggestion here and try to be more flexible, we should be able to handle this case.

Thanks for the suggestion!

bcjonescbt commented 1 year ago

A different direction to go might be to have another option in the command that specified if it was JuptyerLab, Jupyter, or Colab. That's kind of a style question I guess. Maybe it's an optional arg?

jrobinso commented 1 year ago

@bcjonescbt How would that help?

bcjonescbt commented 1 year ago

I was getting at the need the code has to handle the three environments differently: JupyterLab/notebook/Colab. The convertURL function in messageHandler.js has to switch based on whether it is JupyterLab or not, which is what broke for me.

Looking a little more, I see you have const vars isColab and isNotebook in the js module. I guess it would make sense to have a similar isJupyterLab const. But this runs into the issue of trying to divine the JupyterLab status in a very general way. I was thinking of the optional argument as an escape route from trying to guess every users' URL pattern, or if the JupyterLab project changed its URL pattern in the future. If there was perhaps an optional argument to Browser.__init__, maybe the state of isJupyterLab could be explicitly stated by the end user. Then that state could be held internally and end up in the messageHander.js function somehow (I'm not a javascript programmer so I don't know exactly how that would happen).

I'm obviously just guessing here.

jrobinso commented 1 year ago

@bcjonescbt OK thanks, i haven't looked into this in detail yet but yes an explicit parameter makes sense if we are doing things differently between lab and notebook. Its not really needed for colab but wouldn't hurt. I'm just not sure it will solve it. On my local Jupyter using paths relative to the location of the notebook (no leading slash) works reliably, the example notebook is wrong in that respect.

What type of paths are you using in the notebook? Relative to the (no leading slash), or absolute (leading slash)?

I hope to look at this in more detail today, but I think something along the lines of your original suggestion might be the key.

bcjonescbt commented 1 year ago

@jrobinso I have been using absolute paths. Thanks for looking at it.

bcjonescbt commented 1 year ago

Maybe the cleanest (from end user standpoint) solution would be to have an argument like ivg_notebook.init(browser_type = 'JupyterLab'), set once in the session and then handled internally. In contrast, passing the argument to igv_browser.load_track(...) would require setting it every place in the end user code.

Of course, whether it's worth the implementation hassles would be up to you. I don't know javascript so it's not clear to me how a state variable would exist between the two languages.

jrobinso commented 1 year ago

@bcjonescbt I think your original suggestion is the best. Everything is a heuristic here in Jupyter Lab land. Just knowing its JupyterLab doesn't help anything, really, its already known that it is not Colab and it is not Notebook.