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

Shut down unused kernels #16341

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

NexVeridian
Copy link
Contributor

@NexVeridian NexVeridian commented May 17, 2024

References

closes #13916

Code changes

Adds a button to Shut Down Unused kernels

User-facing changes

image

Backwards-incompatible changes

Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@NexVeridian NexVeridian marked this pull request as ready for review May 20, 2024 16:29
@krassowski
Copy link
Member

Thank you for working on this!

  • It looks like there are conflicts which need to be resolved, these seem to prevent CI from running
  • can you add a screenshot to the top-level comment?
  • "only show button in kernels section" - shall we allow IRunningSessions.IManager to contribute arbitrary buttons to the section toolbars?

Copy link
Member

@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.

This is looking better - leaving some more feedback on this iteration.

packages/running-extension/src/kernels.tsx Outdated Show resolved Hide resolved
packages/running/src/index.tsx Outdated Show resolved Hide resolved
packages/running-extension/src/kernels.tsx Outdated Show resolved Hide resolved
packages/running-extension/src/kernels.tsx Outdated Show resolved Hide resolved
packages/running-extension/src/kernels.tsx Show resolved Hide resolved
toolbarButtons: [
new CommandToolbarButton({
commands,
id: SHUTDOWN_UNUSED_BUTTON_CLASS,
Copy link
Member

Choose a reason for hiding this comment

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

From a quick glance the logic looks all good now. The last thing I would suggest is using an icon instead of text for this button. Let's first choose and add an icon to the command. I think some kind of material design icon for cleaning could do?

Suggested change
id: SHUTDOWN_UNUSED_BUTTON_CLASS,
icon: someIcon,
id: SHUTDOWN_UNUSED_BUTTON_CLASS,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I added one, I don't see very many options for icons

I manually added it to /jupyterlab/packages/ui-components/src/icon/iconimports.ts which needs to be fixed since it's supposed to be auto-generated

Should the text for Shut Down All and Shut Down Unused be removed, and just have icons instead, since there's not that much room

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.

Add button closing dangling kernels
2 participants