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 full process info. #65

Merged
merged 5 commits into from
Aug 3, 2019
Merged

Conversation

bethune-bryant
Copy link
Contributor

@bethune-bryant bethune-bryant commented Jun 26, 2019

Fixes #50

I added -f, --show-full-cmd that show the full process info as discussed in #50.

Right now it shows the percent of CPU usage and the percent of system memory in use, but that can be changed.

Let me know what you think.

Example:

server1  Wed Jun 26 15:24:33 2019  418.67
[0] Tesla V100-SXM2-16GB | 34'C,  23 % |  1097 / 16130 MB | user1(1087M)
 └─ 72041 ( 80%,  0.24%): python /mnt/home/user1/git/horovod/examples/keras_mnist.py
[1] Tesla V100-SXM2-16GB | 36'C,  23 % |  1097 / 16130 MB | user1(1087M)
 └─ 72042 ( 80%,  0.24%): python /mnt/home/user1/git/horovod/examples/keras_mnist.py
[2] Tesla V100-SXM2-16GB | 35'C, 100 % |  2130 / 16130 MB | user2(777M) user1(1343M)
 ├─ 95638 (100%,  0.16%): /mnt/home/user2/anaconda3/envs/env/bin/python test_c10d.py
 └─ 72043 (100%,  0.24%): python /mnt/home/user1/git/horovod/examples/keras_mnist.py
[3] Tesla V100-SXM2-16GB | 34'C,  22 % |  1097 / 16130 MB | user1(1087M)
 └─ 72044 ( 40%,  0.24%): python /mnt/home/user1/git/horovod/examples/keras_mnist.py

@wookayin
Copy link
Owner

wookayin commented Jul 9, 2019

Thanks for your contribution. After all CI tests fixed, I'll have a look to merge this. BTW I am debating what would be the best command line flag (rather than -f): one option that comes across is -C (similar to -c), but not fully convinced myself.

@bethune-bryant
Copy link
Contributor Author

Thanks @wookayin

After all CI tests fixed,

Yeah, the unicode characters aren't behaving nicely with Python 2.7. I'm trying to figure that out.

BTW I am debating what would be the best command line flag (rather than -f): one option that comes across is -C (similar to -c), but not fully convinced myself.

I don't have strong feelings one way or another on this -C makes sense.

@bethune-bryant
Copy link
Contributor Author

@wookayin
All the tests are passing. I had to add a python 2 specific line.

Let me know what you decide about the flag and I can make that change.

@Stonesjtu
Copy link
Collaborator

Just FYI.
python2.7 is retiring in Jan 1st 2020, so probably the master branch can focus on only python 3.
Ref. https://pythonclock.org/

@bethune-bryant
Copy link
Contributor Author

Just FYI.
python2.7 is retiring in Jan 1st 2020, so probably the master branch can focus on only python 3.
Ref. https://pythonclock.org/

That makes sense to me. That would definitely simplify these changes.

@Stonesjtu
Copy link
Collaborator

@wookayin
According to https://stackoverflow.com/questions/40114100/uploading-different-versions-python-2-7-vs-3-5-to-pypi, we can simply add python_requires='>=3.4' and bump the version.
Then if you install the package via pip2, the out-dated one instead of the newest version will be selected.

@bethune-bryant
Copy link
Contributor Author

I opened an issue (#66) and can make the Python 2 changes separately before we merge these changes.

@wookayin wookayin added this to the 1.0 milestone Jul 22, 2019
@wookayin
Copy link
Owner

The current master has dropped python2 -- would you want to rebase onto master and drop changesets for python2 support if necessary?

@bethune-bryant
Copy link
Contributor Author

@wookayin
Yeah, I'll make another commit to remove the python2 specific changes.

@bethune-bryant
Copy link
Contributor Author

@wookayin
I rebased this PR and removed the python 2 specific changes.

Let me know if there's anything else you'd like me to change.

Copy link
Owner

@wookayin wookayin left a comment

Choose a reason for hiding this comment

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

I love this feature, and thanks for your contribution.

Other than the line-by-line comments, there is one more thing we need to resolve: it's slow. cpu_percent() needs 0.1 seconds of blocking sampling per process; in a case where we have 64+ processes it will take many seconds. If we can cache and reuse the Process instances then it can be non-blocking via interval=0 (for gpustat -i cases), but not sure for one-time run without loops. Perhaps we can also parallelize using async/await features.

gpustat/test_gpustat.py Outdated Show resolved Hide resolved
gpustat/core.py Outdated Show resolved Hide resolved
@bethune-bryant
Copy link
Contributor Author

If we can cache and reuse the Process instances then it can be non-blocking via interval=0 (for gpustat -i cases), but not sure for one-time run without loops. Perhaps we can also parallelize using async/await features.

I will spend some time figuring out the best way to do this.

@bethune-bryant
Copy link
Contributor Author

@wookayin
I made changes to improve the performance if the cpu usage checking, and it works really well on python 3.7. Apparently the asyncio stuff has changed alot throughout 3.4-3.7, so I'll need to look into it more.

@wookayin
Copy link
Owner

Looks cool. Two comments:

  • In light of getting process information, the gpustat object now requires a context (e.g. pid and process) so it doesn't make any more sense to have a static initializer retrieve all the necessary information (e.g. you had to use static global variables which is not good). But I think this is a matter of refactoring I should take care of -- so let's merge it soon and let me work on this. It is also related to Improve speed by sparingly querying nvml #61.

  • Can we make sure asyncio features work well throughout python 3.4-3.7? Or how can we test it? As it requires a bit complicated mocking so writing a test seems to be a bit tricky. But it'd be great to have a minimal test; later on I will refactor test cases using smarter mocking libraries (mockito).

gpustat/core.py Outdated Show resolved Hide resolved
@bethune-bryant
Copy link
Contributor Author

@wookayin
What do you think? Is there anything else you'd like me to change before we merge?

@wookayin
Copy link
Owner

wookayin commented Aug 2, 2019

Looks good to me, thanks!

@wookayin wookayin merged commit 3975a1f into wookayin:master Aug 3, 2019
@wookayin
Copy link
Owner

wookayin commented Aug 3, 2019

Let's get this merged. I'll add a bit of follow-up commits and cleanups. Thanks a lot for your hard work!

wookayin added a commit that referenced this pull request Aug 3, 2019
- Introduce gpustat.util for misc util functions.
- Use a less attractive color (dark blue-ish) for full process info,
  with basename of the command (as per `-p`) colored differently.
- Small refactoring for better coding practices.
@wookayin
Copy link
Owner

wookayin commented Aug 3, 2019

I made a small changes in a9c27f3.

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.

Show full commands running
3 participants