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

Merge jupyterlab-kernel-usage #163

Merged

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
…9e637c37751efa2'

git-subtree-dir: jupyterlab-kernel-usage
git-subtree-mainline: 74a391c
git-subtree-split: b22e7f8
@davidbrochart
Copy link
Contributor Author

I ran the following commands to get the history from https://github.com/Quansight/jupyterlab-kernel-usage:

git subtree add --prefix=jupyterlab-kernel-usage https://github.com/Quansight/jupyterlab-kernel-usage.git main
git rm -r jupyterlab-kernel-usage
git commit -m "Remove jupyterlab-kernel-usage"

Fix too frequent queries from frontend and too long timeout on backend
@krassowski
Copy link
Collaborator

I merged Quansight/jupyterlab-kernel-usage#24 so that it is easier to include if you would be happy to rebase including that :)

@davidbrochart davidbrochart force-pushed the jupyterlab-kernel-usage branch from b920c20 to 6703e02 Compare December 14, 2022 10:27
@davidbrochart
Copy link
Contributor Author

OK to merge?

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.

I haven't finished reviewing but sending comments-as is to reply to the question above.

setup.cfg Outdated
@@ -49,6 +49,7 @@ include_package_data = True
packages = find:
python_requires = >=3.6
install_requires =
ipykernel >=6.11.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we drop this since this is only required for kernel-usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably yes, but then it should be checked at run-time if kernel-usage is used?

Copy link
Member

Choose a reason for hiding this comment

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

A runtime check sounds appropriate. Some setups might have jupyter-resource-usage installed but without ipykernel and still want the existing memory indicator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix it, but the README says that ipykernel>=6.11.0 is required, while the code checks for ipykernel>6.9.0. Which one is it?

Co-authored-by: Michał Krassowski <5832902+krassowski@users.noreply.github.com>
@@ -0,0 +1,57 @@
// Taken from https://github.com/jupyter-server/jupyter-resource-usage/blob/e6ec53fa69fdb6de8e878974bcff006310658408/packages/labextension/src/memoryUsage.tsx#L272
Copy link
Member

Choose a reason for hiding this comment

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

Are these comments needed now that the files live in the same repo?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like memoryUsage.tsx can now reuse things from format.ts?

Comment on lines 61 to 62
label: 'Kernel Usage',
caption: 'Kernel Usage',
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 nice to pass these strings through the translator.

@davidbrochart
Copy link
Contributor Author

@jtpio @krassowski let me know if 9bfbcb5 is fine. I'm not sure about the translation thing.

@davidbrochart davidbrochart merged commit 8858fcd into jupyter-server:master Dec 15, 2022
@welcome
Copy link

welcome bot commented Dec 15, 2022

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

@davidbrochart
Copy link
Contributor Author

Noooo, I merged the wrong PR!

@davidbrochart
Copy link
Contributor Author

Let me know if there were outstanding changes, and I will open a follow-up PR 😄

@davidbrochart
Copy link
Contributor Author

Actually, since I squashed, I should revert the commit, right?

@davidbrochart
Copy link
Contributor Author

I wanted to force-push to remove the commit, but the branch is protected.
Should we temporarily unprotect it?

@davidbrochart
Copy link
Contributor Author

OK, I removed the protection, force-pushed, and re-created the rule.
I will open the same PR again.

@jtpio
Copy link
Member

jtpio commented Dec 15, 2022

No problem thanks @davidbrochart.

@jtpio
Copy link
Member

jtpio commented Dec 15, 2022

I'm not sure about the translation thing.

Looks like it's addressed in #164 now with the use of the ITranslator.

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.

Merge with jupyter-resource-usage?
4 participants