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

Jupyterlab kernel usage #164

Merged
merged 70 commits into from
Dec 22, 2022

Conversation

davidbrochart
Copy link
Contributor

…not-available

Show a message is kernel usage is not available
…nt-pooling

Pool only the active kernel and if the sidebar is visible
const kernel_extension: JupyterFrontEndPlugin<void> = {
id: '@jupyter-server/resource-usage:kernel-panel-item',
autoStart: true,
requires: [ITranslator, ICommandPalette, INotebookTracker],
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to keep at least ICommandPalette as optional here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Before merging, I should probably squash (not the commits from jupyterlab-kernel-usage of course).

davidbrochart and others added 2 commits December 19, 2022 16:27
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
@davidbrochart davidbrochart force-pushed the jupyterlab-kernel-usage branch 2 times, most recently from ca4ea5b to 5115041 Compare December 19, 2022 15:47
Copy link
Collaborator

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

  1. Works well on binder - thank you!
  2. Translations may be improved, see inline comments; I do not have a strong opinion on whether colon should be included in translation strings.
  3. For translations we will also need a PR adding this extension to repository-map.yml in language-packs repo

".*$",
[
(
url_path_join(base_url, "jupyterlab_kernel_usage", r"get_usage/(.+)$"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to harmonise the endpoint name with the other one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like /api/metrics/kernel_usage/get_usage/{kernel_id}?

Copy link
Member

Choose a reason for hiding this comment

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

Something like sounds good, maybe keeping the v1 as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

packages/labextension/package.json Outdated Show resolved Hide resolved
packages/labextension/src/widget.tsx Outdated Show resolved Hide resolved
packages/labextension/src/widget.tsx Outdated Show resolved Hide resolved
davidbrochart and others added 5 commits December 19, 2022 18:54
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
@davidbrochart
Copy link
Contributor Author

  1. Translations may be improved, see inline comments; I do not have a strong opinion on whether colon should be included in translation strings.
  2. For translations we will also need a PR adding this extension to repository-map.yml in language-packs repo

Done.

setup.cfg Outdated Show resolved Hide resolved
davidbrochart and others added 2 commits December 22, 2022 11:27
Co-authored-by: Jeremy Tuloup <jeremy.tuloup@gmail.com>
Copy link
Collaborator

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@jtpio
Copy link
Member

jtpio commented Dec 22, 2022

Nice thanks both!

@davidbrochart or @krassowski feel free to make a new release with this change if you would like to. The repo uses the releaser already. For the version we could bump to 0.7.0. Otherwise I'm also happy to make the release.

edit: you should have received invites for the PyPI and npm projects

@davidbrochart
Copy link
Contributor Author

I'm seeing that when the kernel is busy executing some code (e.g. while True: pass), the extension shows "Kernel usage details are not available". Is it the normal behavior?

@krassowski
Copy link
Collaborator

krassowski commented Dec 22, 2022

I think this was reported before although I could not reproduce. Might be a regression specific to a new version of ipykernel? In any case I think it's fine to merge and release.

Edit: Sorry for spam, I missed invites, thank you!

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

4 participants