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

Update to JupyterLab 4.3.0b1 #7423

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Update to JupyterLab 4.3.0b1 #7423

wants to merge 12 commits into from

Conversation

@jtpio jtpio added this to the 7.3.0 milestone Jul 11, 2024
Copy link
Contributor

Binder 👈 Launch a Binder on branch jtpio/notebook/update-lab

@jtpio
Copy link
Member Author

jtpio commented Jul 11, 2024

bot please update playwright snapshots

@jtpio jtpio closed this Jul 11, 2024
@jtpio jtpio reopened this Jul 11, 2024
@jtpio jtpio closed this Jul 11, 2024
@jtpio jtpio reopened this Jul 11, 2024
@jtpio
Copy link
Member Author

jtpio commented Jul 12, 2024

Looks like the following error might be relevant:

image

Also need to check the effects of jupyterlab/jupyterlab#16446 on Notebook.

@jtpio
Copy link
Member Author

jtpio commented Jul 12, 2024

Looks like the following error might be relevant:

image

Just noticed an upstream change that might be relevant: jupyterlab/jupyterlab#16523

cc @krassowski who may know more about this. Wondering if this is because we had to fetch the docmanager settings manually at some point due to some race conditions:

// Explicitly get the default viewers from the settings because
// JupyterLab might not have had the time to load the settings yet (race condition)
// Relevant code: https://github.com/jupyterlab/jupyterlab/blob/d56ff811f39b3c10c6d8b6eb27a94624b753eb53/packages/docmanager-extension/src/index.tsx#L265-L293
if (settingRegistry) {
const settings = await settingRegistry.load(
JUPYTERLAB_DOCMANAGER_PLUGIN_ID
);
const defaultViewers = settings.get('defaultViewers').composite as {
[ft: string]: string;
};
// get the file types for the path
const types = docRegistry.getFileTypesForPath(path);
// for each file type, check if there is a default viewer and if it
// is available in the docRegistry. If it is the case, use it as the
// default factory
types.forEach((ft) => {
if (
defaultViewers[ft.name] !== undefined &&
docRegistry.getWidgetFactory(defaultViewers[ft.name])
) {
defaultFactory = defaultViewers[ft.name];
}
});
}

@krassowski
Copy link
Member

Nothing jumps at me here, but some ideas:

  • does adding notebook needs to wait on await settingRegistry.ready // @ts-ignore help?
  • does it make sense to wrap most of the code in settings.changed(() => { /* register if not registered */ })?

@jtpio
Copy link
Member Author

jtpio commented Jul 12, 2024

@krassowski
Copy link
Member

Also seeing this on #7447. I do not think this is 4.3.x specific issue

@krassowski
Copy link
Member

krassowski commented Aug 26, 2024

The error is coming from here:

https://github.com/jupyterlab/jupyterlab/blob/7bae8e3238967a0a776028150b49a4903759bc90/packages/docmanager-extension/src/index.tsx#L276-L282

settings.get('defaultViewers').composite is undefined, hence Object.keys(defaultViewers) throws.

In fact all settings accessed in onSettingsUpdated there are undefined. And this is the initial call to onSettingsUpdated after application was restored.

@krassowski
Copy link
Member

krassowski commented Aug 26, 2024

And that code is referenced in Notebook codebase here:

// Explicitly get the default viewers from the settings because
// JupyterLab might not have had the time to load the settings yet (race condition)
// Relevant code: https://github.com/jupyterlab/jupyterlab/blob/d56ff811f39b3c10c6d8b6eb27a94624b753eb53/packages/docmanager-extension/src/index.tsx#L265-L293
if (settingRegistry) {
const settings = await settingRegistry.load(
JUPYTERLAB_DOCMANAGER_PLUGIN_ID
);
const defaultViewers = settings.get('defaultViewers').composite as {
[ft: string]: string;
};

Maybe there is no race condition but the settings for docmanager plugin are only loaded but not transformed yet. I guess that the issue in JupyterLab is masked by the fact that app.restored is fired much later.

@krassowski
Copy link
Member

I see the same error on released Jupyter Notebook 7.2.0 using https://gist.github.com/jtpio/befb8ebf2b630b4748e2538f79877022

image

I now think this should not be a blocker for merging this one as there is no regression in JupyterLab 4.3 nor 4.2.x as this was just overlooked previously.

@jtpio
Copy link
Member Author

jtpio commented Sep 2, 2024

Thanks @krassowski for looking into this 👍

I guess we could just ignore this for now then.

Spinning back a dev install for this branch, it looks like there is still an issue with the background color though:

image

image

@krassowski
Copy link
Member

an issue with the background color

Does body or other container need to get .jp-ThemedContainer (ref: https://jupyterlab.readthedocs.io/en/latest/extension/extension_migration.html#css-styling / jupyterlab/jupyterlab#16519)?

@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

Done in 40387bb.

Although not seeing any change for now:

image

@jtpio jtpio changed the title Update to JupyterLab 4.3.0a2 Update to JupyterLab 4.3.0b1 Sep 6, 2024
@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

Is there a way to reduce the size of the "File size" column? I guess it's related to jupyterlab/jupyterlab#16646 ?

image

@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

Although not seeing any change for now:

This change of background color is also picked up in the UI tests, so it's not a local dev environment issue only:

image

@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

This change of background color is also picked up in the UI tests, so it's not a local dev environment issue only:

It looks like .jp-ThemedContainer was taking precedence over the custom rule defined here in Notebook:

background: var(--jp-layout-color2);

image

@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

Arf, looks like there are more CSS regressions to fix, for example the menus are now gray and there is no styling on hover:

image

Comment on lines 13 to 22
body {
margin: 0;
padding: 0;
}

/*
Override the default background
See https://github.com/jupyterlab/jupyterlab/pull/16519 for more information
*/
.jp-ThemedContainer {
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 this is because of this change. I think the right fix is to just adjust the selector to body.jp-ThemedContainer:

Suggested change
body {
margin: 0;
padding: 0;
}
/*
Override the default background
See https://github.com/jupyterlab/jupyterlab/pull/16519 for more information
*/
.jp-ThemedContainer {
body.jp-ThemedContainer {
margin: 0;
padding: 0;

Copy link
Member

Choose a reason for hiding this comment

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

This will also make embedding notebook in other websites easier (when using a full javascript rather than iframe) as the styles will not conflict :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, tried that before but it didn't seem to be enough:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like #main.jp-ThemedContainer also had to be defined.

@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

Is there a way to reduce the size of the "File size" column? I guess it's related to jupyterlab/jupyterlab#16646 ?

I guess it's because Notebook 7 does not provide support for workspaces and restoring the file browser? Maybe we should hardcode some default values that match the previous defaults for now?

@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

@krassowski any suggestion for setting proper defaults for the file browser column sizes? I tried something like the following but it didn't seem to have any effect:

stateDB.save('file-browser-filebrowser:columns', {
  sizes: {
    name: 258.125,
    file_size: 60,
    is_selected: 18,
    last_modified: 778.899513036933,
  },
})
.then(async () => {
  await browser['listing'].restore('filebrowser');
});

@jtpio
Copy link
Member Author

jtpio commented Sep 6, 2024

Or maybe this will be made easier with jupyterlab/jupyterlab#16751, so we can just specify custom CSS directly instead.

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

Successfully merging this pull request may close these issues.

2 participants