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

Attempted fix for minor JupyterHub compatibility issue. #6931

Merged
merged 6 commits into from Aug 15, 2019
Merged

Attempted fix for minor JupyterHub compatibility issue. #6931

merged 6 commits into from Aug 15, 2019

Conversation

@krinsman
Copy link
Contributor

@krinsman krinsman commented Aug 2, 2019

This is an attempt to address the TODO in /packages/hub-extension/src/index.ts.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Aug 2, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@rcthomas
Copy link

@rcthomas rcthomas commented Aug 2, 2019

We actually rely on named servers in our JupyterHub deployment, so this is actually presenting an issue right now.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 2, 2019

Do we know how widespread jupyterhub 1.0 is? This seems like it demands jhub 1.0, so we might not be able to do it until jupyterlab 2.0 for backwards compatibility. Is there a way to support jupyterhub 1.0 and previous jupyterhub?

@jasongrout jasongrout added this to the 2.0 milestone Aug 2, 2019
@@ -211,6 +211,7 @@ def load_jupyter_server_extension(nbapp):
page_config['hubPrefix'] = nbapp.hub_prefix
page_config['hubHost'] = nbapp.hub_host
page_config['hubUser'] = nbapp.user
page_config['hubServerName'] = nbapp.server_name
Copy link
Member

@blink1073 blink1073 Aug 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a guard here for hasattr(nbapp, 'server_name')

Copy link
Contributor Author

@krinsman krinsman Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that the default server name is the empty string (as @rcthomas said). If URLExt.join is smart and ignores empty strings as a no-op then presumably such a switch would be unnecessary. That being said I am not sure.

Copy link
Contributor Author

@krinsman krinsman Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@krinsman krinsman Aug 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the issue is that the server wouldn't have that attribute if it was just a single person running JupyterLab on their laptop, then why aren't guards hasattr(nbapp, 'hub_prefix'), hasattr(nbapp, 'hub_host'), and hasattr(nbapp, 'user') also necessary? I'm a little confused.

If the issue is that it might not have that attribute when JupyterHub is being run, I don't think that's possible although I guess better safe than sorry.

Copy link
Member

@blink1073 blink1073 Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant is to check whether the property exists as a way to detect whether we are using jupyterhub 1.0 or not.

Copy link
Contributor Author

@krinsman krinsman Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright then it seems like it might not be necessary since the server_name attribute existed pre-1.0. jupyterhub/jupyterhub#2089

I think it was called name before my PR (although I'm not 100%), but it was in master at least as early as September 2018.

Copy link
Contributor

@jasongrout jasongrout Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was in master at least as early as September 2018.

FYI, it looks like that PR was merged and released starting with 1.0 (jupyterhub/jupyterhub@f2fa067 - see the tags that contain that commit)

Copy link
Contributor Author

@krinsman krinsman Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasongrout Oh oops, you're right. I'm not very good with understanding how the versioning for these things work. Just because it was merged doesn't mean it was in the current most recent release for that time. It does make sense, since the UI for named servers only showed up with 1.0 and not any earlier versions.

Now I feel dumb. I thought changing the name from name to server_name would make the code easier to understand/work with, but that seems to not have been the case. Anyway @blink1073 is definitely right about needing the hasattr. Sorry about that; I think I'm getting overly defensive/insecure about the changes I made/proposed.

const restartUrl =
hubHost + URLExt.join(hubPrefix, `spawn?next=${hubPrefix}home`);
hubHost +
Copy link
Member

@blink1073 blink1073 Aug 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we switch here on whether there is a server name and choose the simplified url if there isn't one? Does the simplified url work in jupyterhub 1.0?

This comment has been hidden.

Copy link
Contributor Author

@krinsman krinsman Aug 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope sorry I think I was wrong: https://github.com/jupyterhub/jupyterhub/blob/1a26c1fb817f44acffd3907d0ec94d7de115c2e6/jupyterhub/handlers/pages.py#L131
You're right that switch is the right approach as far as I can tell.

@rcthomas
Copy link

@rcthomas rcthomas commented Aug 2, 2019

@jasongrout The fix also needs to work for JupyterHub>=1 where the JupyterHub config doesn't have named servers turned on (it's off by default). One way of looking at it is that the default server already has a name, it's just "" (empty string).

We're going to try to get a test set up of this hopefully today and I'm thinking we'll test against JupyterHub 1 with named servers turned off to make sure everything is right. That said, what version of Hub would be late enough for compatibility? 0.9.6? Thanks!

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 2, 2019

My big concern is that people may have deployed jlab 1.0 already to jhub 0.x, and it stops working in a minor update to jlab. What version of jhub do we support currently? 0.9.0 was released a year ago - is there any difference in supporting 0.9.0 vs 0.9.6? A year seems like a reasonable support timeline.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 2, 2019

(We should make our support for a specific version of jhub explicit in the docs somewhere, and perhaps even in the setup.py and/or conda recipe if we can)

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 2, 2019

what I had suggested would support both pre and post 1.0 of jupyterhub, but I agree in principle about being explicit.

tslaton added a commit to tslaton/jupyterlab that referenced this issue Aug 3, 2019
@krinsman
Copy link
Contributor Author

@krinsman krinsman commented Aug 5, 2019

I am not sure if JupyterHub compatibility is actually an issue, since JupyterHub has supported named servers via an API (and thus the URLs) for a while, with the default server name just being the empty string (e.g. when named servers are turned off or none exist). The only change in 1.0 is the existence of a UI for named servers. jupyterhub/jupyterhub#2089

That having been said the important thing for me to do right now first would be to test this to make sure it works the way I expect/think it does.

EDIT: See this line in the JupyterHub source -- the server_name attribute always exists, and is the empty string by default: https://github.com/jupyterhub/jupyterhub/blob/master/jupyterhub/singleuser.py#L449

@krinsman
Copy link
Contributor Author

@krinsman krinsman commented Aug 5, 2019

How does URLExt.join work? If say token2 = '', i.e. the second/"middle" string is the empty string, does it return token1//token3 or token1/token3?

I.e. what is URLExt.join('token1', '', 'token3')?

My guess is that there'd be a problem with the PR in the former case but not necessarily the latter.

@tslaton
Copy link
Collaborator

@tslaton tslaton commented Aug 6, 2019

@krinsman was reporting some issues on NERSC's deployment running this PR that I wasn't seeing on my separate fork. I think the only differences between this PR and my fork were that I'd added the hasattr guard and restart url switch @blink1073 suggested on my fork.

Now that I have gained push rights here, I went ahead and implemented those changes so that at least @krinsman and I are on the same page. I still think it's worth considering his comments above questioning what makes these changes necessary.

EDIT: To be clearer, he was reporting trouble launching lab to begin with, whereas I have only seen trouble with server restarts (which these changes have yet to remedy)

@blink1073 blink1073 removed this from the 2.0 milestone Aug 6, 2019
@blink1073 blink1073 added this to the 1.1 milestone Aug 6, 2019
@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 6, 2019

If y'all get it working with both JupyterHub 1.0 and pre-1.0, I'll make a backport release of JupyterLab 1.0 with this change. Thanks for working on this!

@blink1073 blink1073 self-assigned this Aug 6, 2019
@tslaton
Copy link
Collaborator

@tslaton tslaton commented Aug 13, 2019

I think there were some bugs @rcthomas resolved in jupyterhub/jupyterhub#2682 and issues with our setup at NERSC that were making it difficult to test the behavior of the restart flow due to the JupyerLab code in isolation before.

Now that I am able to play with it, I don't think the following code is quite right (as proposed):

  const restartUrl = hubServerName
	    ? hubHost + URLExt.join(
	        hubPrefix,
	        'spawn',
	        hubUser,
	        hubServerName,
	        `?next=${hubPrefix}home`
	      )
	    : hubHost + URLExt.join(hubPrefix, `spawn?next=${hubPrefix}home`);

When hubServerName is '', the fallback url is working as intended. But when there is a hubServerName, I'm getting a 404 error when navigating to restartUrl.

Removing ?next=${hubPrefix}home seems to fix the 404 error and give the behavior I was expecting to see (the named server that has stopped restarts in a new tab). So, I think the correct code should be:

  const restartUrl = hubServerName
	    ? hubHost + URLExt.join(
	        hubPrefix,
	        'spawn',
	        hubUser,
	        hubServerName
	      )
	    : hubHost + URLExt.join(hubPrefix, `spawn?next=${hubPrefix}home`);

I don't yet have a good understanding of why we might want to keep the next portion in the url when using named servers, so I'm willing to accept viewpoints as to why this isn't correct here. Otherwise, I can make this small change to the PR, and I think it'd be ready to go.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 14, 2019

What is the behavior with a named server when you do not specify next? It seems like you'd want to go to the home of that specific named server. If that is the behavior with the proposed change, I say let's go for it.

@krinsman
Copy link
Contributor Author

@krinsman krinsman commented Aug 14, 2019

@tslaton This sounds right to me -- I added the last ?next=... part mostly because I didn't remember how all of the hub redirects work, and so copying it from the nameless server case seemed the safest/most conservative thing.

Have you added/merged those changes into the PR?

This page seems to explain it the best, although doesn't really address the next query argument.
https://github.com/jupyterhub/jupyterhub/blob/071e375d5fd038e70c29c619d199bf2c9c090d5c/docs/source/reference/urls.md#hubuserusernameservername
Probably a relevant quote for this issue:

Prior to 1.0, this URL (/hub/user/:username[/:servername]) itself was responsible for spawning servers, and served the progress page if it was pending, redirected to running servers, and This was useful because it made sure that requested servers were restarted after they stopped, but could also be harmful because unused servers would continuously be restarted if e.g. an idle JupyterLab frontend were open pointed at it, which constantly makes polling requests.
Special handling of API requests
Requests to /user/:username[/:servername]/api/... are assumed to be from applications connected to stopped servers. These are failed with 503 and an informative JSON error message indicating how to spawn the server. This is meant to help applications such as JupyterLab that are connected to a server that has stopped.
Version changed: 1.0
JupyterHub 0.9 failed these API requests with status 404, but 1.0 uses 503.

EDIT: Actually probably the more relevant portion of the page for our case:
https://github.com/jupyterhub/jupyterhub/blob/071e375d5fd038e70c29c619d199bf2c9c090d5c/docs/source/reference/urls.md#hubspawnusernameservername

/hub/spawn[/:username[/:servername]]
Requesting /hub/spawn will spawn the default server for the current user. If username and optionally servername are specified, then the specified server for the specified user will be spawned. Once spawn has been requested, the browser is redirected to /hub/spawn-pending/....
If Spawner.options_form is used, this will render a form, and a POST request will trigger the actual spawn and redirect.
Version added: 1.0
1.0 adds the ability to specify username and servername. Prior to 1.0, only /hub/spawn was recognized for the default server.
Version changed: 1.0
Prior to 1.0, this page redirected back to /hub/user/:username, which was responsible for triggering spawn and rendering progress, etc.

I think the big takeaway here would be that the routing/redirects actually are substantially different between pre- and post-1.0, so it's understandable why this has been more difficult than expected. (At least I personally had incorrectly assumed that they were largely the same.)

@tslaton
Copy link
Collaborator

@tslaton tslaton commented Aug 14, 2019

Case 1: Restarting a named server with ?next specified as above opens the "spawn pending" page in a new tab, which redirects to the hub home (as specified with ?next) when spawning is finished. At the hub home, the restarted named server is visible (as would be any other named servers on that hub) and clicking on the named server gets into JupyterLab.

I had been getting a 404 error attempting this previously, but that was due to an extra / at the end of the url before the ?next portion. It's easy to remedy.

Case 2: Restarting a named server without ?next as specified above opens the "spawn pending" page in a new tab, which shows the JupyterLab instance corresponding to the restarted named server immediately when spawning is finished (no need to click into it from the hub home).

I think the second case is the behavior I'd expect to see.

EDIT: The changes required to the PR if we desire Case 1 behavior are simply removing an extra / in the restartUrl. The changes required if we desire Case 2 behavior are dropping the whole ?next portion from the restartUrl. I can add the appropriate changes once we've settled on which of the two behaviors is desired. Although I expect Case 2 personally, it sounds like @blink1073 expects Case 1. Case 1 is also most consistent with the fallback url. We should probably be consistent about redirecting to the hub home in the named servers case and in the fallback case, regardless of which one is chosen.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 14, 2019

I think it is fair to have different behavior between pre-1.0 and post-1.0. I actually agree with Case 2 as the post-1.0 behavior. I am restarting this named server when I hit restart. Otherwise I'd log out and start a different named server.

@tslaton
Copy link
Collaborator

@tslaton tslaton commented Aug 14, 2019

Unless the failing tests pose an issue (not sure they're related to this PR), it should be ready now. It follows the Case 2 behavior: when restarting a named server, the user will not be redirected to the hub home.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 15, 2019

LGTM, thanks!

@blink1073 blink1073 merged commit 2a7ba40 into jupyterlab:master Aug 15, 2019
7 of 9 checks passed
@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 15, 2019

@meeseeksdev please backport to 1.0.x

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 15, 2019

@meeseeksdev backport to 1.0.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this issue Aug 15, 2019
blink1073 added a commit that referenced this issue Aug 15, 2019
…1-on-1.0.x

Backport PR #6931 on branch 1.0.x (Attempted fix for minor JupyterHub compatibility issue.)
@lock lock bot locked as resolved and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants