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

Adding support for jupyterlab statusbar-extension #36 #45

Merged
merged 9 commits into from
Jun 12, 2020
Merged

Adding support for jupyterlab statusbar-extension #36 #45

merged 9 commits into from
Jun 12, 2020

Conversation

Gsbreddy
Copy link

@Gsbreddy Gsbreddy commented Jun 11, 2020

This solves yuvipanda#36

Have to raise PR on jupyterlab statusbar-extension to change api call to /api/nbresuse/v1 once this PR is merged.

Once this is done, then we can merge yuvipanda#41 and release a new version to PyPi.

  • Add back psutil as a dependency
  • Add back the /metrics endpoint with mem and cpu usage

@jtpio
Copy link
Member

jtpio commented Jun 11, 2020

How about something like the following timeline?

Users with JupyterLab <= 2.x will need nbresuse=0.3.3
Users with JupyterLab >= 3.x (or the version including the new endpoint) will need nbresuse==0.5+

The main issue is to be able to deal with all the breaking changes and what is currently implemented in core JupyterLab.

cc @krinsman if you have any input on this

@Gsbreddy
Copy link
Author

LGTM.

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

@Gsbreddy I'm pushing a couple of fixes to:

Also CI doesn't seem to be triggered, fixing that separately.

@Gsbreddy
Copy link
Author

What about the /metrics endpoint? Won't this be a problem as you told here. #36 (comment)

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

Yes, but since we can't yank 0.3.4 at the moment the only option seems to be adding the endpoint back, and releasing 0.3.5. So it's still compatible with older clients for a little while.

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

Once this is done, then we can merge #41 and release a new version to PyPi.

We can probably do that separately, especially if we bring back /metrics.

We can also bring back the CPU metrics to the API endpoint since it was available in 0.3.3.

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

We can also bring back the CPU metrics to the API endpoint since it was available in 0.3.3.

Will push a commit to handle that.

@jtpio jtpio marked this pull request as draft June 12, 2020 11:04
@jtpio jtpio self-assigned this Jun 12, 2020
@Gsbreddy
Copy link
Author

This won't be useful for core lab though. Just the notebook.

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

This extension depends on it: https://github.com/jtpio/jupyterlab-system-monitor

Also if it's gone it's another breaking change.

@Gsbreddy
Copy link
Author

Okay. Is there any plan to merge https://github.com/jtpio/jupyterlab-system-monitor to core lab?

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

Not really, although this could be considered if more people are using it.

We would first need to decide on the future of nbresuse and the scope it should cover. So that we can build and expose a stable API.

@jtpio jtpio marked this pull request as ready for review June 12, 2020 11:51
@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

@Gsbreddy let me know if you want to try the latest from this branch locally.

There are no tests at the moment but we should eventually add some (#49).

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

Also what do you think about removing the /api/nbresuse/v1 endpoint for now?

Since it's a new endpoint, there could be a good opportunity to design the API with what we plan to add next in mind (for example kernel metrics).

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

Merging, we can iterate in separate PRs.

@jtpio jtpio merged commit 7fb8112 into jupyter-server:master Jun 12, 2020
@jtpio jtpio deleted the new-api-for-lab branch June 12, 2020 12:41
@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

Thanks again @Gsbreddy for this!

@Gsbreddy
Copy link
Author

Sure! We can add this endpoint later with the design which has more information like per kernel memory and cpu usages. Looking forward to help on this.

@jtpio
Copy link
Member

jtpio commented Jun 12, 2020

That's great, thanks @Gsbreddy!

(I now have commit rights on the repo, so we can keep some momentum)

@yuvipanda
Copy link
Collaborator

Hello! I appreciate all the fixes here, @jtpio and @Gsbreddy!!!

I've opened #68 with a 'soft deprecation' pathway for the /metrics endpoint, enabling admins to opt-in to removing /metrics for now. Would love y'all taking a look :)

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

Successfully merging this pull request may close these issues.

3 participants