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.

Sign up for GitHub

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

Jump to bottom

Add a new flag --no-processes to hide process information #133

Merged
merged 8 commits into from
Oct 15, 2022
Merged

Add a new flag --no-processes to hide process information #133

merged 8 commits into from
Oct 15, 2022

Conversation

doncamilom
Copy link
Contributor

Hey I find this useful for my own application, also seems to solve #101 I think :)

@wookayin
Copy link
Owner

Hello, thanks for your contribution!

`-n`, `--no-processes`    : Hide memory data for individual processes.

Actually the name of the flag doesn't fully explain or match its description. It more sounds like it would completely disable the process information.

FYI, this can be also seen as a special case of #51 (more general solution) -- users may want to have more fine-grained control of process output; we may want to avoid having tons of command-line flags.

@doncamilom
Copy link
Contributor Author

Hi, thanks for your response.
The whole point of the flag is indeed to disable process information.

In practice I would go from something like:

[0] NVIDIA GeForce RTX 3090 | 65'C, 100 % | 14672 / 24576 MB | andres(1755M) andres(1755M) andres(1755M) andres(1755M) andres(1757M) gdm(35M) dalco(37M) dalco(4M) andres(153M) andres(18M) andres(4M) andres(9M) andres(4M)

To

[0] NVIDIA GeForce RTX 3090 | 66°C, 100 % | 14672 / 24576 MB

Which is exactly what I needed :)

Has #51 been implemented yet tho?

Thank you for your work with this repo btw, very useful! ^^

@wookayin
Copy link
Owner

I see. So it tries to hide all the process information. Suggestions:

  • I think it'd better to not add the shorthand -n flag, i.e. let's use --no-processes only.
  • Hide memory data for individual processes. this description doesn't match your intention. Can you please fix the documentation?
  • Please update README.md, the usage section accordingly.
  • I would prefer not adding the | separator at the end of line, when process information is hidden.
  • Please add tests, in test_args_commandline. (see https://github.com/wookayin/gpustat/blob/master/gpustat/test_gpustat.py#L308)

@wookayin wookayin added this to the 1.1 milestone Sep 13, 2022
@doncamilom
Feature: Hide all process information
1334485 
- Update README
- added test in `test_args_commandline`
- Fixed output string format
@doncamilom
Copy link
Contributor Author

Thank you for your review!
Hope the documentation is better now.

wookayin
wookayin reviewed Sep 14, 2022
View reviewed changes
gpustat/test_gpustat.py Outdated Show resolved Hide resolved
wookayin
wookayin reviewed Sep 14, 2022
View reviewed changes
gpustat/core.py Outdated Show resolved Hide resolved
doncamilom and others added 2 commits September 14, 2022 23:07
@doncamilom
Fixed test and output string for flag --no-processes.
38289ae 
      - Not Supported only added if flag is not used
      - Trailing " |" not added if flag not used.
@doncamilom
Merge branch 'wookayin:master' into master
4c3b6f3
wookayin
wookayin reviewed Oct 10, 2022
View reviewed changes
gpustat/core.py Outdated Show resolved Hide resolved
wookayin
wookayin requested changes Oct 10, 2022
View reviewed changes
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.

We've got some conflicts. Can you please move some arguments and squash all the commits without merge-with-up-stream merge-commits? Thank you.

@wookayin wookayin changed the title Feature: Add flag to hide memory usage of individual processes Add a new flag --no-processes to hide process information Oct 15, 2022
@wookayin wookayin merged commit af7d760 into wookayin:master Oct 15, 2022
@wookayin
Copy link
Owner

Thank you, I've merged this.

wookayin added a commit that referenced this pull request Oct 15, 2022
@wookayin
Improve documentation for #133 and add a changelog entry.
34069c6
@wookayin wookayin mentioned this pull request Dec 1, 2022
Closed
@wookayin wookayin mentioned this pull request Dec 15, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wookayin wookayin wookayin approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
2 participants
@doncamilom @wookayin