Add support for enc/dec gpu utilization (#79) #80

ChaoticMind · 2020-03-19T08:58:56Z

This PR only adds encoder and decoder utilization to --json from the cmdline, or to the GPUStat object if gpustat is used as a library.

The information is also exposed to the standard command line output via the -e or --show-codec flag

See issue #79

Stonesjtu · 2020-03-19T09:55:27Z

But Travis gives a passed result. Can you also try to add the cli interface?

ChaoticMind · 2020-03-19T10:05:35Z

~~Maybe it was because I ran them through SSH which messed with the encoding (stripping ansi?):~~

- [0] GeForce GTX TITAN 0 | 80'C,  76 % |  8000 / 12287 MB | user1(4000M) user2(4000M)
                              ^
+ [0] GeForce GTX TITAN 0 | 80°C,  76 % |  8000 / 12287 MB | user1(4000M) user2(4000M)
                              ^

Edit: It was because the tests were importing the system version of gpustat and therefore there was a mismatch 😕 - Will now run it in a venv instead 😀

Ok, will look into the cli interface soon. Do you think it should be hidden behind a flag or always exposed?

ChaoticMind · 2020-03-19T11:49:19Z

@Stonesjtu I hid the encoder/decoder info behind a -e flag. It takes -e enc, -e dec, or -e enc,dec (default).

The functionality seems to be working well, however the test is currently failing, and I'm not exactly sure why:

>           if "enc" in show_encoder and "dec" in show_encoder:
E           TypeError: argument of type 'bool' is not iterable

Even though show_encoder is of type strand notbool, and the code is working for all parameters. Any idea what's up?

Another minor thing is that I expected the strings "enc" and "dec" in gpustat -e to be bold (because of CBold), but they don't seem to be properly bolded on my terminal.

Stonesjtu

Plz see comments

gpustat/cli.py

Stonesjtu

Plz fix the failed test.

gpustat/core.py

gpustat/test_gpustat.py

wookayin

Thanks for the PR. I added some comments.

gpustat/cli.py

gpustat/core.py

gpustat/test_gpustat.py

Stonesjtu · 2020-03-20T16:01:54Z

Can you modify second paragraph of the pr message because you have added cli output feature.

Stonesjtu · 2020-03-21T06:36:55Z

gpustat/core.py

@@ -221,6 +249,14 @@ def _repr(v, none_value='??'):
            reps += "%(FSpeed)s{entry[fan.speed]:>3} %%%(C0)s, "

        reps += "%(CUtil)s{entry[utilization.gpu]:>3} %%%(C0)s"
+        if "enc" in show_codec:


I think it's good to wrap them with parenthesis when --show-codec is specified, like the first version.

Ok, I added the parenthesis back.

Stonesjtu

The PR looks good overall.

ChaoticMind · 2020-03-21T12:24:45Z

I added the parenthesis back and force pushed.

Stonesjtu

You can append encoder_line and decoder_line in to a list and then ' '.join(a_certain_ilst)

ChaoticMind · 2020-03-21T12:50:02Z

You can append encoder_line and decoder_line in to a list and then ' '.join(a_certain_ilst)

Do you mean, instead of:

if "enc" in show_codec and "dec" in show_codec:
    reps += encoder_line + "  " + decoder_line
elif "enc" in show_codec:
    reps += encoder_line
elif "dec" in show_codec:
    reps += decoder_line

Something like:

if "enc" in show_codec and "dec" in show_codec:
    reps += "  ".join([encoder_line, decoder_line])
elif "enc" in show_codec:
    reps += encoder_line
elif "dec" in show_codec:
    reps += decoder_line

I'd still need to have three separate if/elif clauses. I think it's less readable, no?

Stonesjtu · 2020-03-21T13:27:53Z

I mean:

codec_fields = []
if "enc" in show_codec:
    codec_fields.append(encoder_field)
if "dec" in show_codec:
    codec_fields.append(decoder_field)

reps += " ".join(codec_fields)

I think this should be easier to follow.

BTW: I would rather not calling them encoder/decoder line because they are not lines.

ChaoticMind · 2020-03-21T16:19:18Z

I'm happy with the PR now. If we want to be extra picky:

Does -e as short flag for --show-codec make sense? The flag was originally called --show-encoder. -c is already taken, but maybe -C?
Should I add a line to CHANGELOG.md
colors['CBold'] doesn't seem to actually make the E: and D: characters bold for some reason, at least on my terminal.

wookayin · 2020-03-22T04:36:05Z

@ChaoticMind Overall LGTM, thanks. I will update CHANGELOG and do a bit more of cleanup myself. I am supportive of -C, but need to think more. Actually -e sounds good. I am also considering using -C for #65. Any opinions are welcome.

For bold text, you may need to configure your terminal to draw bold text in bold font. For example, iTerm2 has such an option (Preferences > Text > Text Rendering).

Stonesjtu

Lgtm

Stonesjtu · 2020-03-22T15:14:16Z

@wookayin Actually we don't have to give every option an abbr. It's enough to cover the most frequently used ones.

Stonesjtu · 2020-03-24T04:51:37Z

@wookayin can you approve the PR so we can merge it.

wookayin

LGTM! Thanks for the contribution.

Add support for enc/dec gpu utilization (wookayin#79)

f20fd98

ChaoticMind mentioned this pull request Mar 19, 2020

Expose GPU enc/dec utilization #79

Closed

ChaoticMind force-pushed the enc_dec_util branch 2 times, most recently from 42be639 to 0b287e5 Compare March 19, 2020 11:37

Stonesjtu requested changes Mar 19, 2020

View reviewed changes

gpustat/cli.py Show resolved Hide resolved

ChaoticMind force-pushed the enc_dec_util branch 2 times, most recently from aa92b4c to 09963d5 Compare March 19, 2020 13:11

ChaoticMind requested a review from Stonesjtu March 19, 2020 13:14

Stonesjtu requested changes Mar 20, 2020

View reviewed changes

gpustat/core.py Outdated Show resolved Hide resolved

gpustat/test_gpustat.py Outdated Show resolved Hide resolved

wookayin requested changes Mar 20, 2020

View reviewed changes

gpustat/cli.py Outdated Show resolved Hide resolved

gpustat/core.py Outdated Show resolved Hide resolved

gpustat/test_gpustat.py Outdated Show resolved Hide resolved

ChaoticMind force-pushed the enc_dec_util branch from 09963d5 to dfc2ecb Compare March 20, 2020 14:50

ChaoticMind requested review from Stonesjtu and wookayin March 20, 2020 14:54

Stonesjtu reviewed Mar 21, 2020

View reviewed changes

ChaoticMind force-pushed the enc_dec_util branch from dfc2ecb to e9b9381 Compare March 21, 2020 12:18

ChaoticMind requested a review from Stonesjtu March 21, 2020 12:21

Stonesjtu reviewed Mar 21, 2020

View reviewed changes

Add command-line support for enc/dec gpu utilization

59c979a

ChaoticMind force-pushed the enc_dec_util branch from e9b9381 to 59c979a Compare March 21, 2020 16:10

ChaoticMind requested a review from Stonesjtu March 21, 2020 16:12

wookayin added this to the 1.0 milestone Mar 22, 2020

wookayin added the new feature label Mar 22, 2020

Stonesjtu approved these changes Mar 22, 2020

View reviewed changes

wookayin approved these changes Mar 24, 2020

View reviewed changes

wookayin merged commit b80d8e8 into wookayin:master Mar 24, 2020

wookayin added a commit that referenced this pull request Mar 24, 2020

Update changelog for the --show-codec feature (#79, #80)

09930c5

ChaoticMind deleted the enc_dec_util branch April 5, 2020 05:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for enc/dec gpu utilization (#79) #80

Add support for enc/dec gpu utilization (#79) #80

ChaoticMind commented Mar 19, 2020 •

edited

Stonesjtu commented Mar 19, 2020

ChaoticMind commented Mar 19, 2020 •

edited

ChaoticMind commented Mar 19, 2020

Stonesjtu left a comment

Stonesjtu left a comment

wookayin left a comment •

edited

Stonesjtu commented Mar 20, 2020

Stonesjtu Mar 21, 2020

ChaoticMind Mar 21, 2020

Stonesjtu left a comment

ChaoticMind commented Mar 21, 2020

Stonesjtu left a comment

ChaoticMind commented Mar 21, 2020 •

edited

Stonesjtu commented Mar 21, 2020

ChaoticMind commented Mar 21, 2020

wookayin commented Mar 22, 2020 •

edited

Stonesjtu left a comment

Stonesjtu commented Mar 22, 2020

Stonesjtu commented Mar 24, 2020

wookayin left a comment •

edited

Add support for enc/dec gpu utilization (#79) #80

Add support for enc/dec gpu utilization (#79) #80

Conversation

ChaoticMind commented Mar 19, 2020 • edited

Stonesjtu commented Mar 19, 2020

ChaoticMind commented Mar 19, 2020 • edited

ChaoticMind commented Mar 19, 2020

Stonesjtu left a comment

Choose a reason for hiding this comment

Stonesjtu left a comment

Choose a reason for hiding this comment

wookayin left a comment • edited

Choose a reason for hiding this comment

Stonesjtu commented Mar 20, 2020

Stonesjtu Mar 21, 2020

Choose a reason for hiding this comment

ChaoticMind Mar 21, 2020

Choose a reason for hiding this comment

Stonesjtu left a comment

Choose a reason for hiding this comment

ChaoticMind commented Mar 21, 2020

Stonesjtu left a comment

Choose a reason for hiding this comment

ChaoticMind commented Mar 21, 2020 • edited

Stonesjtu commented Mar 21, 2020

ChaoticMind commented Mar 21, 2020

wookayin commented Mar 22, 2020 • edited

Stonesjtu left a comment

Choose a reason for hiding this comment

Stonesjtu commented Mar 22, 2020

Stonesjtu commented Mar 24, 2020

wookayin left a comment • edited

Choose a reason for hiding this comment

ChaoticMind commented Mar 19, 2020 •

edited

ChaoticMind commented Mar 19, 2020 •

edited

wookayin left a comment •

edited

ChaoticMind commented Mar 21, 2020 •

edited

wookayin commented Mar 22, 2020 •

edited

wookayin left a comment •

edited