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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b750683
Attempted fix for minor JupyterHub compatibility issue.
krinsman Aug 2, 2019
f32a57e
Implement Steven Silvester's suggestions #6931
tslaton Aug 6, 2019
fcc893c
Add clarifying comments
tslaton Aug 7, 2019
b4f5d2c
Merge remote-tracking branch 'upstream/master'
tslaton Aug 9, 2019
d9f7b9e
Remove ?next from restartUrl for JupyterHub >=1.0
tslaton Aug 14, 2019
e3644c9
Merge remote-tracking branch 'upstream/master'
tslaton Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions jupyterlab/extension.py
View file
Open in desktop
Expand Up @@ -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

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

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

Choose a reason for hiding this comment

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

See here: https://github.com/jupyterhub/jupyterhub/blob/master/jupyterhub/singleuser.py#L449

Copy link
Contributor Author

@krinsman krinsman Aug 5, 2019 •
edited

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

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

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

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 •
edited

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.

api_token = os.getenv('JUPYTERHUB_API_TOKEN', '')
page_config['token'] = api_token

Expand Down
2 changes: 2 additions & 0 deletions packages/application/src/frontend.ts
View file
Open in desktop
Expand Up @@ -274,6 +274,8 @@ export namespace JupyterFrontEnd {
readonly workspaces: string;
readonly hubPrefix?: string;
readonly hubHost?: string;
readonly hubUser?: string;
readonly hubServerName?: string;
};

/**
Expand Down
4 changes: 3 additions & 1 deletion packages/application/src/lab.ts
View file
Open in desktop
Expand Up @@ -246,7 +246,9 @@ export namespace JupyterLab {
tree: PageConfig.getOption('treeUrl'),
workspaces: PageConfig.getOption('workspacesUrl'),
hubHost: PageConfig.getOption('hubHost') || undefined,
hubPrefix: PageConfig.getOption('hubPrefix') || undefined
hubPrefix: PageConfig.getOption('hubPrefix') || undefined,
hubUser: PageConfig.getOption('hubUser') || undefined,
hubServerName: PageConfig.getOption('hubServerName') || undefined
},
directories: {
appSettings: PageConfig.getOption('appSettingsDir'),
Expand Down
15 changes: 11 additions & 4 deletions packages/hub-extension/src/index.ts
View file
Open in desktop
Expand Up @@ -42,6 +42,8 @@ function activateHubExtension(
): void {
const hubHost = paths.urls.hubHost || '';
const hubPrefix = paths.urls.hubPrefix || '';
const hubUser = paths.urls.hubUser || '';
const hubServerName = paths.urls.hubServerName || '';
const baseUrl = paths.urls.base;

// Bail if not running on JupyterHub.
Expand All @@ -56,11 +58,16 @@ function activateHubExtension(

const { commands } = app;

// TODO: use /spawn/:user/:name
// but that requires jupyterhub 1.0
// and jupyterlab to pass username, servername to PageConfig
// requires jupyterhub 1.0
const restartUrl =
hubHost + URLExt.join(hubPrefix, `spawn?next=${hubPrefix}home`);
hubHost +
Copy link
Member

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 was marked as outdated.

Sign in to view

Copy link
Contributor Author

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.

URLExt.join(
hubPrefix,
`spawn`,
hubUser,
hubServerName,
`?next=${hubPrefix}home`
);

commands.addCommand(CommandIDs.restart, {
label: 'Restart Server',
Expand Down