Add gpu proc info #1276

jamartin9 · 2023-08-18T12:01:19Z

Description

Consolidated gpu calls into data_harvester.

Changed config flag from enable_gpu_memory to enable_gpu.

~~Added GPU utilization to the cpu widget.~~

Added GPU process memory usage and utilization percentage to the proc widget. (enable by uncommenting the process column headers in the config file; not shown by default)
Added key binds for gpu process toggling C M (better defaults?)

~~Added GPU power usage to the battery widget. (linux only as windows returned 0 for me.)~~
Added bounds check to battery widget header (selected out of bounds if clicking across the top of battery widget from left->right; regardless of tabs).
~~Show battery widget header (for gpu name) when enable_gpu. (does gpu power usage belong in the battery widget?)~~
Added battery config/cli precedence

Added feature flag legacy-functions to nvml-wrapper

updated config file(s)
updated help text
updated docs

Screenshots:

linux basic:

linux:

freebsd:

windows basic:

Issues

Maybe closes: #566 and #1291(disable temp with gpu flag) ~~and #298 unless more information is desired (like fan speed for 272). Should there be a list of things still needed? (amd/intel gpus?)~~

NVML seems inconsistent at times. Linux/Windows results differed after driver updates. (maybe keep legacy-functions feature until issue 48 resolved in nvml-wrapper...)

Testing

Ran on windows(w/gpu), linux (w/gpu) and freebsd (no gpu)

Windows
macOS
Linux
FreeBSD

Checklist

Areas your change affects have been linted using rustfmt (cargo fmt)
The change has been tested and doesn't appear to cause any unintended breakage
Documentation has been added/updated if needed (README.md, help menu, doc pages, etc.)
The pull request passes the provided CI pipeline
There are no merge conflicts
If relevant, new tests were added (don't worry too much about coverage)

I did NOT test on macOS.

If any of the Issues should be addressed or a different approach used please let me know.

Feel free to close this PR if it is unwanted.

Consolidated gpu calls into `data_harvester`. Changed config flag from `enable_gpu_memory` to `enable_gpu`. Added GPU utilization to the cpu widget. Added GPU process memory usage and utilization percentage to the proc widget. Added key binds for gpu process toggling. Added GPU power usage to the battery widget. Added bounds check to battery widget header. Show battery widget header when `gpu_enable`. Added feature flag `legacy-functions` to `nvml-wrapper`. updated config file(s). updated help text. updated docs.

codecov · 2023-08-18T12:14:03Z

Codecov Report

Attention: 350 lines in your changes are missing coverage. Please review.

Comparison is base (7c14aa2) 31.82% compared to head (4177bc2) 31.57%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
src/app/data_harvester/nvidia.rs	0.00%	97 Missing ⚠️
src/app/query.rs	0.00%	80 Missing ⚠️
src/app/data_harvester.rs	0.00%	33 Missing ⚠️
src/app.rs	0.00%	26 Missing ⚠️
src/app/data_harvester/processes/windows.rs	0.00%	22 Missing ⚠️
src/app/data_harvester/processes/linux.rs	0.00%	21 Missing ⚠️
src/widgets/process_table.rs	31.03%	20 Missing ⚠️
src/widgets/process_table/proc_widget_column.rs	0.00%	17 Missing ⚠️
src/widgets/process_table/proc_widget_data.rs	0.00%	17 Missing ⚠️
src/app/data_harvester/processes.rs	0.00%	6 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1276      +/-   ##
==========================================
- Coverage   31.82%   31.57%   -0.26%     
==========================================
  Files          98       97       -1     
  Lines       16674    16943     +269     
==========================================
+ Hits         5307     5349      +42     
- Misses      11367    11594     +227

Flag	Coverage Δ
macos-12	`33.33% <15.19%> (-0.24%)`	⬇️
ubuntu-latest	`33.14% <14.36%> (-0.26%)`	⬇️
windows-2019	`33.36% <14.32%> (-0.29%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ClementTsang

Sorry for taking so long - been busy with some other stuff.

Generally, I like what is done and do want to merge this; adding GPU support has been something I've wanted for a while. However, I feel like there's a few areas where I would rather GPU information be displayed (at least by default) as a separate widget, like usage and power consumption.

Feel free to let me know if there's any areas you would like to see changes in the code to make that easier.

src/app/data_harvester.rs

docs/content/usage/widgets/process.md

src/app/data_harvester.rs

src/app/query.rs

jamartin9 · 2023-08-31T01:52:06Z

Thank you for your comments and feedback thus far.

Please take your time and apologizes for the large PR.

there's a few areas where I would rather GPU information be displayed (at least by default) as a separate widget, like usage and power consumption.

With removal of the power/util the only added information in this PR is for gpu procs. I have updated the description with the changes.

Feel free to let me know if there's any areas you would like to see changes in the code to make that easier.

I have not looked into adding widgets yet. My initial concern is screen space/layout in advanced mode (I mostly use basic mode).
I will probably not create a gpu widget right away; given that the new gpu widget will need to display graphs/time data for util, power, fan speed etc. (Hence the addition to the current displays.) The aesthetic choices should be made by someone more inclined to do so... (unless it is maybe just be one big graph of 0-100% with real values in the legend?)

Remove GPU util from cpu widget Remove GPU power from battery widget Use reference for gpu widgets_to_harvest Extract match arm to function for feature gate

ClementTsang · 2023-10-08T20:40:16Z

Sorry for the delay with reviewing this btw, been really with work/life for a bit - will get to looking at this in a bit.

0re5ama · 2023-11-04T07:53:30Z

Any Progress on this?

…gpu-utils

ClementTsang · 2023-11-14T18:41:02Z

Sorry, just got back from a vacation 😅 , will look at this sometime this week.

…gpu-utils

ClementTsang

Looks mostly good to me, apologies for taking so long to review! Just a few comments/questions/nitpicks but bulk of it seems fine.

docs/content/configuration/config-file/processes.md

src/app.rs

src/app/data_harvester.rs

src/app/data_harvester/processes/linux.rs

src/app/data_harvester/temperature.rs

src/args.rs

src/options.rs

src/widgets/process_table/proc_widget_column.rs

adjust doc wordings remove extra references remove extra widget harvest checks init proc gpu values use convert_temp_unit for gpu temp

ClementTsang

Small nitpicks, but otherwise looks good to me. Happy to merge if you don't have anything else to add in.

ClementTsang · 2023-11-20T04:34:45Z

src/widgets/process_table/proc_widget_column.rs

+            ProcColumn::GpuMem => "GMEM",
+            #[cfg(feature = "gpu")]
+            ProcColumn::GpuMemPercent => "GMEM%",


Nit for consistency:

Suggested change

ProcColumn::GpuMem => "GMEM",

#[cfg(feature = "gpu")]

ProcColumn::GpuMemPercent => "GMEM%",

ProcColumn::GpuMem => "GMem",

#[cfg(feature = "gpu")]

ProcColumn::GpuMemPercent => "GMem%",

ClementTsang · 2023-11-20T04:34:58Z

src/widgets/process_table/proc_widget_column.rs

+            #[cfg(feature = "gpu")]
+            ProcColumn::GpuMem => "GMEM",
+            #[cfg(feature = "gpu")]
+            ProcColumn::GpuMemPercent => "GMEM%",


Same here:

Suggested change

#[cfg(feature = "gpu")]

ProcColumn::GpuMem => "GMEM",

#[cfg(feature = "gpu")]

ProcColumn::GpuMemPercent => "GMEM%",

#[cfg(feature = "gpu")]

ProcColumn::GpuMem => "GMem",

#[cfg(feature = "gpu")]

ProcColumn::GpuMemPercent => "GMem%",

ClementTsang · 2023-11-20T04:35:46Z

src/options.rs

@@ -433,7 +433,7 @@ pub fn build_app(
        use_cpu: used_widget_set.get(&Cpu).is_some() || used_widget_set.get(&BasicCpu).is_some(),
        use_mem,
        use_cache: use_mem && get_enable_cache_memory(matches, config),
-        use_gpu: use_mem && get_enable_gpu_memory(matches, config),
+        use_gpu: use_mem && get_enable_gpu(matches, config),


Can probably just do:

Suggested change

use_gpu: use_mem && get_enable_gpu(matches, config),

use_gpu: get_enable_gpu(matches, config),

jamartin9 · 2023-11-20T04:50:24Z

Small nitpicks, but otherwise looks good to me. Happy to merge if you don't have anything else to add in.

I do not have anything to add unless you want me to address the comments. I can assist in testing after the merge for the changes too.

Thanks again for the responses.

ClementTsang · 2023-11-20T04:53:46Z

They're small things that I can address quickly after merging this, so no worries there.

And testing would be appreciated! I don't have any computers that use NVIDIA GPUs at the moment so it's a bit hard for me to test right now unless I mock it or until I look into supporting other GPUs.

actionless · 2024-01-02T06:42:38Z

all widgets (like processes, memory or temp) work as expected but on this one i'm not getting GPU0 etc, do i need to enable some config option for that?

jamartin9 · 2024-01-02T07:48:16Z

all widgets (like processes, memory or temp) work as expected but on this one i'm not getting GPU0 etc, do i need to enable some config option for that?

It was determined during code review (commit) to remove the utilization and power consumption from the default displays per the following comment:

I would rather GPU information be displayed as a separate widget, like usage and power consumption.

I didn't update the screenshots after removal. Sorry for the confusion.

actionless · 2024-01-02T08:43:55Z

and that separate widget is implemented or not yet?

actionless · 2024-01-02T08:45:22Z

btw i also find it a bit confusing showing VRAM and RAM memory in the same widget, but split GPU and CPU processors - in that case i guess VRAM also should be moved to the gpu widget?

jamartin9 · 2024-01-02T09:15:12Z

and that separate widget is implemented or not yet?

It is not implemented yet. Anyone is free to implement it tho.

VRAM also should be moved to the gpu widget?

Most likely imo

ClementTsang self-assigned this Aug 18, 2023

ClementTsang requested changes Aug 30, 2023

View reviewed changes

merge master

084259c

jamartin9 changed the title ~~Add gpu util, power and procs.~~ Add gpu info Aug 31, 2023

jamartin9 added 2 commits August 31, 2023 08:42

Code Review:

4234ec2

Remove GPU util from cpu widget Remove GPU power from battery widget Use reference for gpu widgets_to_harvest Extract match arm to function for feature gate

Code Review: add gmem% toggle

dbbc267

jamartin9 requested a review from ClementTsang August 31, 2023 16:34

jamartin9 added 2 commits August 31, 2023 12:51

Do not poll gpu temp when filtered

6809d69

Merge branch 'master' into gpu-utils

2daaf58

Merge branch 'master' of https://github.com/ClementTsang/bottom into …

0c31380

…gpu-utils

jamartin9 added 2 commits November 17, 2023 13:10

Merge branch 'master' of https://github.com/ClementTsang/bottom into …

194ba43

…gpu-utils

Merge branch 'master' into gpu-utils

807e3c2

ClementTsang reviewed Nov 19, 2023

View reviewed changes

jamartin9 added 3 commits November 19, 2023 08:00

Merge remote-tracking branch 'upstream/master' into gpu-utils

54c0624

Code Review Two Changes:

ce93091

adjust doc wordings remove extra references remove extra widget harvest checks init proc gpu values use convert_temp_unit for gpu temp

Merge branch 'gpu-utils' of github.com:jamartin9/bottom into gpu-utils

4177bc2

jamartin9 changed the title ~~Add gpu info~~ Add gpu proc info Nov 19, 2023

jamartin9 requested a review from ClementTsang November 19, 2023 23:51

ClementTsang approved these changes Nov 20, 2023

View reviewed changes

ClementTsang merged commit e4a6e75 into ClementTsang:master Nov 20, 2023
33 checks passed

ClementTsang mentioned this pull request Nov 20, 2023

change: small gpu proc changes #1325

Merged

9 tasks

jamartin9 mentioned this pull request Nov 27, 2023

GPU and VRAM load #298

Open

BrewTestBot mentioned this pull request Aug 1, 2024

bottom 0.10.0 Homebrew/homebrew-core#179202

Merged

shurizzle mentioned this pull request Aug 13, 2024

No GPU temps (Nvidia) after updating to 0.10 #1554

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gpu proc info #1276

Add gpu proc info #1276

jamartin9 commented Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading

ClementTsang left a comment

jamartin9 commented Aug 31, 2023 •

edited

Loading

ClementTsang commented Oct 8, 2023

0re5ama commented Nov 4, 2023

ClementTsang commented Nov 14, 2023

ClementTsang left a comment •

edited

Loading

ClementTsang left a comment

ClementTsang Nov 20, 2023

ClementTsang Nov 20, 2023

ClementTsang Nov 20, 2023

jamartin9 commented Nov 20, 2023

ClementTsang commented Nov 20, 2023

actionless commented Jan 2, 2024

jamartin9 commented Jan 2, 2024

actionless commented Jan 2, 2024

actionless commented Jan 2, 2024 •

edited

Loading

jamartin9 commented Jan 2, 2024 •

edited

Loading

	use_gpu: use_mem && get_enable_gpu(matches, config),
	use_gpu: get_enable_gpu(matches, config),

Add gpu proc info #1276

Add gpu proc info #1276

Conversation

jamartin9 commented Aug 18, 2023 • edited Loading

Description

Issues

Testing

Checklist

codecov bot commented Aug 18, 2023 • edited Loading

Codecov Report

ClementTsang left a comment

Choose a reason for hiding this comment

jamartin9 commented Aug 31, 2023 • edited Loading

ClementTsang commented Oct 8, 2023

0re5ama commented Nov 4, 2023

ClementTsang commented Nov 14, 2023

ClementTsang left a comment • edited Loading

Choose a reason for hiding this comment

ClementTsang left a comment

Choose a reason for hiding this comment

ClementTsang Nov 20, 2023

Choose a reason for hiding this comment

ClementTsang Nov 20, 2023

Choose a reason for hiding this comment

ClementTsang Nov 20, 2023

Choose a reason for hiding this comment

jamartin9 commented Nov 20, 2023

ClementTsang commented Nov 20, 2023

actionless commented Jan 2, 2024

jamartin9 commented Jan 2, 2024

actionless commented Jan 2, 2024

actionless commented Jan 2, 2024 • edited Loading

jamartin9 commented Jan 2, 2024 • edited Loading

jamartin9 commented Aug 18, 2023 •

edited

Loading

codecov bot commented Aug 18, 2023 •

edited

Loading

jamartin9 commented Aug 31, 2023 •

edited

Loading

ClementTsang left a comment •

edited

Loading

actionless commented Jan 2, 2024 •

edited

Loading

jamartin9 commented Jan 2, 2024 •

edited

Loading