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.0rc0 #7423

Merged
merged 32 commits into from
Oct 14, 2024
Merged

Update to JupyterLab 4.3.0rc0 #7423

merged 32 commits into from
Oct 14, 2024

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

@jtpio jtpio closed this Oct 7, 2024
@jtpio jtpio reopened this Oct 7, 2024
@jtpio
Copy link
Member Author

jtpio commented Oct 7, 2024

bot please update playwright snapshots

@jtpio
Copy link
Member Author

jtpio commented Oct 14, 2024

bot please update playwright snapshots

@jtpio jtpio closed this Oct 14, 2024
@jtpio jtpio reopened this Oct 14, 2024
@jtpio jtpio changed the title Update to JupyterLab 4.3.0b3 Update to JupyterLab 4.3.0rc0 Oct 14, 2024
@jtpio
Copy link
Member Author

jtpio commented Oct 14, 2024

Arf, it looks like the file browser columns sizes are still a bit unpredictable? Which seems to be introducing flakiness to the UI tests:

filebrowser-flakiness.webm

@jtpio
Copy link
Member Author

jtpio commented Oct 14, 2024

For example a first load of the page would give the following:

image

The next page reload:

image

@jtpio
Copy link
Member Author

jtpio commented Oct 14, 2024

@krassowski @JasonWeill is there something we are missing here to get consistent and predictable widths for the different file browser columns?

@jtpio
Copy link
Member Author

jtpio commented Oct 14, 2024

So JupyterLab sets some defaults here:

https://github.com/jupyterlab/jupyterlab/blob/c321c27b50431fc0414b20a3b61375af7ccb6c2e/packages/filebrowser/style/base.css#L173-L186

But the file browser columns appear to have the width style set on page load anyway:

image

@jtpio jtpio marked this pull request as ready for review October 14, 2024 19:56
@jtpio
Copy link
Member Author

jtpio commented Oct 14, 2024

OK giving up for now on the file browser thing. Will open a separate issue to track it.

So we can finally get this one in a pre-release!

@jtpio jtpio merged commit 324de44 into jupyter:main Oct 14, 2024
31 checks passed
@jtpio jtpio deleted the update-lab branch October 14, 2024 19:57
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