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

Add section about CPU usage in the README #29

Closed
jtpio opened this issue Apr 3, 2020 · 7 comments · Fixed by #30
Closed

Add section about CPU usage in the README #29

jtpio opened this issue Apr 3, 2020 · 7 comments · Fixed by #30

Comments

@jtpio
Copy link
Member

jtpio commented Apr 3, 2020

Just like for the memory usage, there could be a section in the README file that demonstrates how to specify the options.

Also it looks CPU is not tracked by default, which could be confusing if a user only sets cpu_limit:

https://github.com/yuvipanda/nbresuse/blob/783735980c8dc09907499fb4a1bd401b8d22adf8/nbresuse/__init__.py#L72-L77

Was there a reason to make cpu usage tracking an opt-in option?

Thanks!

@jtpio
Copy link
Member Author

jtpio commented Apr 3, 2020

Was there a reason to make cpu usage tracking an opt-in option?

Ah that might be because the default notebook extension doesn't show this metric yet?

@krinsman
Copy link
Contributor

krinsman commented Apr 3, 2020

Yes, these are both good recommendations. I agree that I should have updated the documentation to mention this feature, as well as to state explicitly that an option needs to be added to the notebook config to enable it.

Yes, part of the reason for not making the CPU usage tracked by default is because it isn't displayed in the NBExtension included in this package, and a JupyterLab extension is required to display it in JupyterLab https://github.com/NERSC/jupyterlab-cpustatus (Also unfortunately that JupyterLab extension appears to be out-of-date as of February due to the release of JupyterLab 2.0. NERSC/jupyterlab-cpustatus#3)

Also since the CPU tracking feature was added recently and not by Yuvi (i.e. by me, who is not nearly as skilled as Yuvi), in other words it was/is somewhat experimental, my recollection was that Yuvi thought (and I agreed) it would be safest to not enable it by default, so as to ensure backwards compatibility/avoid possibly breaking NBResuse for users who don't use the CPU tracking feature.

Since the code for tracking the memory usage with JupyterLab is in JupyterLab master/core, in terms of long-term goals for the project, I would imagine at least three steps:

(1) a PR to JupyterLab to add the CPU display ability by default, not just the memory display, i.e. "merge" this repo into JupyterLab master https://github.com/NERSC/jupyterlab-cpustatus
(2) Make PSUtil an "optional" dependency of NBResuse (i.e. one can have NBResuse installed without it breaking anything if PSUtil isn't installed -- the result will just be that NBResuse does nothing) #25
(3) Merge the NBResuse notebook server extension into jupyter-server (since that presumably will be the backend for JupyterLab in the distant future, but at present is able to be more experimental).

(2) makes (3) possible without adding PSUtil as a required dependency for either JupyterLab or jupyter-server.

Right now I honestly speaking don't have the knowledge to do (1) or (3) or the time available to spend to acquire that knowledge. I have started some work on (2) but I need to dedicate more time towards debugging it/investigating it.

If you're interested in seeing any of this accomplished, I will try to block off 2-3 hours sometime over the next two weeks to look into (2).

@jtpio
Copy link
Member Author

jtpio commented Apr 3, 2020

Thanks for the detailed answer!

Yes, part of the reason for not making the CPU usage tracked by default is because it isn't displayed in the NBExtension included in this package, and a JupyterLab extension is required to display it in JupyterLab https://github.com/NERSC/jupyterlab-cpustatus (Also unfortunately that JupyterLab extension appears to be out-of-date as of February due to the release of JupyterLab 2.0. NERSC/jupyterlab-cpustatus#3)

Also since the CPU tracking feature was added recently and not by Yuvi (i.e. by me, who is not nearly as skilled as Yuvi), in other words it was/is somewhat experimental, my recollection was that Yuvi thought (and I agreed) it would be safest to not enable it by default, so as to ensure backwards compatibility/avoid possibly breaking NBResuse for users who don't use the CPU tracking feature.

OK this sounds like the safest for now. Although cpu tracking mostly adds new attributes to the response payload, it is possible that some users expect a specific schema for the response or don't want this metric to be shared.
Also it could probably be enabled by default in a future version, maybe when bumping to 1.x.

Indeed there is already a memory item in the core status bar. Also the logic doesn't need to change to add CPU metrics since it's the same request to /metrics. So (1) should be feasible without too much effort.

(3) sounds like a good idea in the long run.

As a side note, I maintain a JupyterLab extension that displays these metrics in the top area: https://github.com/jtpio/jupyterlab-system-monitor
It's a different frontend for nbresuse that gives a more "colorful" view on the current metrics.
Today I started added the CPU indicator as well, and merge it very soon: jtpio/jupyterlab-system-monitor#20.
Both indicators share the same model / data (one request to retrieve both the memory and cpu stats). But this required rewriting some of the logic already in core lab. So (1) would indeed be very useful so other frontends can reuse the logic of the model to build alternative views.

Maybe we should join efforts on this!

@krinsman
Copy link
Contributor

krinsman commented Apr 10, 2020

@jtpio Oh wow thank you so much for doing all of this! I actually never imagined anyone else would have been interested enough in the CPU tracking feature I added to update their extensions to use it! To the extent I have time to dedicate towards maintaining NBResuse (which admittedly is not a lot), I would be very happy to join efforts on this.

We were wondering at NERSC whether or not to enable your extension (specifically the topbar part https://github.com/jtpio/jupyterlab-topbar ) by default for all users. One thing some users wanted to have a graphical interface for tracking their memory and CPU usage, which is why I made this (now very out of date) JupyterLab extension several months ago https://github.com/NERSC/jupyterlab_resuse and why we made jupyterlab-cpustatus https://github.com/NERSC/jupyterlab-cpustatus .

Since your extension already has a graphical portion built-in, and already supports the latest version of JupyterLab, and already has support for CPU tracking built-in, it seems like we should even more seriously consider installing it for all users system-wide.

In any case though thank you so much for updating your extension to use this new feature of NBResuse! I'm really grateful -- I honestly thought no one outside of NERSC would have ever noticed or used the new feature.

@krinsman
Copy link
Contributor

One of the TODO's in system-monitor is to also monitor network I/O, which seems to go with this PR Yuvi made a while ago: #19

The basic idea, as far as I understand, is to develop a common interface for any possible resource, instead of needing to reinvent the wheel every time (e.g. like I did with the CPU monitoring). It is perhaps now out-dated since one of its stated aims is to avoid conflict with Prometheus metrics, but now NBResuse uses Prometheus.

To be honest though I am not sure how this could be implemented, but it sounds like you have some ideas. If you come up with something you want to share, please feel free to open a PR which I will probably merge. I might even talk to Yuvi about transferring the repo over to you if you want (he wants to transfer it to someone since he doesn't have time to maintain it anymore), since you seem to be very good at maintaining/updating projects.

@jtpio
Copy link
Member Author

jtpio commented Apr 15, 2020

We were wondering at NERSC whether or not to enable your extension (specifically the topbar part https://github.com/jtpio/jupyterlab-topbar ) by default for all users. One thing some users wanted to have a graphical interface for tracking their memory and CPU usage, which is why I made this (now very out of date) JupyterLab extension several months ago https://github.com/NERSC/jupyterlab_resuse and why we made jupyterlab-cpustatus https://github.com/NERSC/jupyterlab-cpustatus .

Since your extension already has a graphical portion built-in, and already supports the latest version of JupyterLab, and already has support for CPU tracking built-in, it seems like we should even more seriously consider installing it for all users system-wide.

These are really nice extensions!

It looks like there is indeed some overlap between https://github.com/NERSC/jupyterlab_resuse and https://github.com/jtpio/jupyterlab-system-monitor. For the system-monitor extension I would also like to add a panel with more information so it's not only in the top bar. The panel could contain different views for the metrics and more details.

It's tracked in jtpio/jupyterlab-system-monitor#24, and it's also why I'm interested in #31 :)

In any case though thank you so much for updating your extension to use this new feature of NBResuse! I'm really grateful -- I honestly thought no one outside of NERSC would have ever noticed or used the new feature.

Many thanks to you and anyone involved for the work on nbresuse!

The basic idea, as far as I understand, is to develop a common interface for any possible resource, instead of needing to reinvent the wheel every time (e.g. like I did with the CPU monitoring). It is perhaps now out-dated since one of its stated aims is to avoid conflict with Prometheus metrics, but now NBResuse uses Prometheus.

I haven't thought too much about it, but psutil should be helpful for that too: https://psutil.readthedocs.io/en/latest/#psutil.net_io_counters

Having a more generic approach could indeed be useful if we want nbresuse to expose what psutil supports: https://psutil.readthedocs.io/en/latest/#system-related-functions

@krinsman
Copy link
Contributor

Yes, exactly! For example, along with the network I/O, other metrics we would like to be able to keep track of at NERSC include total memory being used on the system (i.e. by all users), and total available memory.

Also the suggestion to limit resource tracking to information from psutil is a really good idea (which admittedly I hadn't thought of before). Writing code that is general enough to be able to track any attribute of the psutil Process class is a much more concrete and attainable goal than what I had in mind, but still accomplishes basically the same goal: https://psutil.readthedocs.io/en/latest/#psutil.Process

Then maybe NBResuse could come with a default list of those attributes/methods to track by default which could then be a configuration option

And then there would probably need to be a separate interface for tracking "system-wide" information using psutil, like network i/o or total memory usage https://psutil.readthedocs.io/en/latest/#psutil.Process
Same thing, with default settings and configurability.

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

Successfully merging a pull request may close this issue.

2 participants