Add functionality to list connected gpu(s) #140

Rolv-Apneseth · 2023-03-15T00:59:36Z

PR is in relation to this issue

So I wasn't sure about where to have the code, how to split it up etc. so please point out where I can improve but I have this working with macchina to output my GPU (on Linux only atm). Theoretically it should output multiple if they're connected but I have no way of testing that.

Also let me know if I should write tests for my code and if so how should I go about doing that, the doc test for the trait doesn't really do anything.

Once this is through I can also make a pr for macchina but my working branch, minus the Cargo.toml changes, is here.

Here is a screenshot of the working GPU output:

src/lib.rs

grtcdr · 2023-03-15T12:40:51Z

src/linux/pci_devices.rs

+        let first_pair = class_value.chars().take(2).collect::<String>();
+
+        match db.classes.get(&first_pair) {
+            Some(class) => class.name == "Display controller",


I believe this is a fragile and expensive comparison. Can we instead match the classes based on their hexadecimal value?

How would I do that? db.classes appears to be a hash map of String to Class I think

Ah, I wasn't aware. I'll have to read up on the documentation of this new library. It might take me some time to really digest the information and provide a decent review.

I'll have a look at the other library I linked originally as it may be better anyway since it bundles the pci.ids

Please don't.

A bundled database means that it could fall out of date at any point in time and that'll affect the information we provide to the caller.

Ok makes sense. Any suggestions on what I could try then since the hashmap is built using strings as keys? Would you prefer I tried making a PR for that library to change it before we use it here?

I don't really mind it, but I'd be interetested to know why the author opted for the corresponding value rather than the short hexadecimal values assigned to them.

I just used hyperfine and reading the GPU is taking quite a while.

Without showing GPU:

Time (mean ± σ): 4.2 ms ± 0.1 ms [User: 3.0 ms, System: 1.2 ms] Range (min … max): 4.0 ms … 4.7 ms 500 runs

With:

Time (mean ± σ): 17.8 ms ± 0.3 ms [User: 14.6 ms, System: 3.1 ms] Range (min … max): 17.0 ms … 19.0 ms 500 runs

Is this acceptable as is?

It's reasonable and expected; it's parsing a 36000-line database after all.

We'll have to implement a caching mechanism at some point, whether that goes in libmacchina or macchina is another story, but I assume it's the client or consumer, generally speaking, that needs to cache the output and not the library.

Alright cool. So then is there anything else you would like from this PR? I can have a look at making a PR for that library to use maybe u8 for the keys if you think that would help? Also are there any changes to documentation / testing I should make here

src/linux/pci_devices.rs

src/traits.rs

Rolv-Apneseth · 2023-03-17T13:16:38Z

Hmm, what should I do about the failing tests? They're lint errors saying it's missing the gpus function.

grtcdr · 2023-03-17T14:20:23Z

Since no other platform besides Linux currently implements the gpu function, you should, for every one of these platforms, implement a function that simply returns Err(ReadoutError::MetricNotAvailable).

Rolv-Apneseth · 2023-03-17T15:08:22Z

Since no other platform besides Linux currently implements the gpu function, you should, for every one of these platforms, implement a function that simply returns Err(ReadoutError::MetricNotAvailable).

Ok thanks, will do

… implemented error

grtcdr · 2023-03-17T17:39:43Z

This has the potential to introduce a panic when the corresponding entry of a PCI device does not exist in the pci.ids database.

thread 'main' panicked at 'Could not find sub device for id: SubDeviceId { subvendor: "17aa", subdevice: "3f0a" }', /home/grtcdr/software/projects/libmacchina/src/linux/pci_devices.rs:98:18
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

We should perhaps omit these devices instead of panicking.

Rolv-Apneseth · 2023-03-17T18:26:43Z

Ok I can skip the device. Also maybe I should return a ReadoutError if the gpus vec is empty after going through the devices?

grtcdr · 2023-03-17T18:28:39Z

Ok I can skip the device.

Great thanks!

Also maybe I should return a ReadoutError if the gpus vec is empty after going through the devices?

Yes, that fits in well with how we usually handle errors and it makes sense in this case as well.

Rolv-Apneseth · 2023-03-17T18:34:00Z

Ah sorry, which one should I use, MetricNotAvailable?

Edit: Never mind sorry I see all the others are using ReadoutError::Other

grtcdr · 2023-03-17T18:50:45Z

I think MetricNotAvailable is appropriate if the host doesn't have any GPUs.

Rolv-Apneseth · 2023-03-17T18:52:38Z

Should the following code still panic? This is before checking the specific sub device. If not, how would I just return None from the function if a value isn't matched, without multiple levels of nesting? I'm till having to find out what the best practices for Rust are for handling Option and Result types.

        let vendor = match db.vendors.get(&vendor_value) {
            Some(vendor) => vendor,
            _ => panic!("Could not find vendor for value: {}", vendor_value),
        };

        let device = match vendor.devices.get(&device_value) {
            Some(device) => device,
            _ => panic!("Could not find device for value: {}", device_value),
        };

Edit: I did this, I believe it works as intended? Lmk if not:

        let Some(vendor) = db.vendors.get(&vendor_value) else {
            return None;
        };

        let Some(device) = vendor.devices.get(&device_value) else {
            return None;
        };

Rolv-Apneseth · 2023-03-17T18:54:19Z

I think MetricNotAvailable is appropriate if the host doesn't have any GPUs.

Makes sense

… error if gpus vec is empty after checking all pci devices

grtcdr · 2023-03-19T12:19:14Z

The last commit uses an unstable feature which, although really cool, will not compile using a stable compiler.

.../macchina $ cargo build                                                  
   Compiling libmacchina v6.3.5 (/home/grtcdr/software/projects/libmacchina)
error[E0658]: `let...else` statements are unstable
  --> /home/grtcdr/software/projects/libmacchina/src/linux/pci_devices.rs:74:9
   |
74 | /         let Some(device) = vendor.devices.get(&device_value) else {
75 | |             return None;
76 | |         };
   | |__________^
   |
   = note: see issue #87335 <https://github.com/rust-lang/rust/issues/87335> for more information

Consider replacing it with a statement such as this one:

diff --git a/src/linux/pci_devices.rs b/src/linux/pci_devices.rs
index 788ebfa..1b19625 100644
--- a/src/linux/pci_devices.rs
+++ b/src/linux/pci_devices.rs
@@ -67,12 +67,14 @@ impl PciDevice {
         let device_value = self._read_value(PciDeviceReadableValues::Device);
         let sub_device_value = self._read_value(PciDeviceReadableValues::SubDevice);
 
-        let Some(vendor) = db.vendors.get(&vendor_value) else {
-            return None;
+        let vendor = match db.vendors.get(&vendor_value) {
+            Some(v) => v,
+            None => return None,
         };
 
-        let Some(device) = vendor.devices.get(&device_value) else {
-            return None;
+        let device = match vendor.devices.get(&device_value) {
+            Some(d) => d,
+            None => return None,
         };
 
         let sub_device_id = SubDeviceId {

Rolv-Apneseth · 2023-03-19T14:43:32Z

The last commit uses an unstable feature which, although really cool, will not compile using a stable compiler.

It's brand new to me and I only used it cause I saw the linter suggesting it to me, but I believe after looking it up it is a stable feature since rustc v1.65.0, at least according to here and here, and compiled fine on my machine, and I believe I only have stable installed. I can still change it if you would like though, not too much difference.

I didn't know the return could be used like that from inside match statements too, still learning exactly how the scopes work to be honest.

grtcdr · 2023-03-19T15:22:44Z

It's brand new to me and I only used it cause I saw the linter suggesting it to me, but I believe after looking it up it is a stable feature since rustc v1.65.0, at least according to here and here, and compiled fine on my machine, and I believe I only have stable installed. I can still change it if you would like though, not too much difference.

I'm on 1.63.0, my bad. I've strayed away from Rust the past year so much so that I'm behind on five whole versions. You should keep it as is.

I didn't know the return could be used like that from inside match statements too, still learning exactly how the scopes work to be honest.

You're right, it's not exactly intuitive.

Rolv-Apneseth · 2023-03-19T19:07:37Z

Is that issue with the Windows CI a problem with the code or what do you think?

grtcdr · 2023-03-20T01:04:22Z

I'll rerun it, looks like it got stuck and timed out.

grtcdr · 2023-03-20T01:24:59Z

I really appreciate the work you've done in this pull request, thank you for the contribution!

Rolv-Apneseth · 2023-03-20T18:39:15Z

No worries, happy to help improve this tool. I'll open a pr soon over on macchina to actually show this info if that's alright, unless you wanted implemented in more than just Linux before I proceed with that

Edit: Oh actually how would that work with the version of libmacchina?

Rolv-Apneseth added 2 commits March 15, 2023 00:46

feat - add functionality to list connected gpu(s)

4a1d1d7

fix - try to fix some lint errors pointed out by workflow

af6844a

grtcdr reviewed Mar 15, 2023

View reviewed changes

Rolv-Apneseth added 2 commits March 16, 2023 22:53

change - remove gpu readout trait and locally define path to pci devices

e1b488b

include list_gpus function in GeneralReadout trait

83cde41

grtcdr reviewed Mar 17, 2023

View reviewed changes

src/traits.rs Outdated Show resolved Hide resolved

Rolv-Apneseth added 2 commits March 17, 2023 12:47

rename list_gpus to gpu

91e2e52

rename list_gpus to gpu

2068884

define gpus function for all OSes other than Linux that returns a not…

aac8a8d

… implemented error

skip devices which don't match any value in the pci.ids db and return…

ece67ac

… error if gpus vec is empty after checking all pci devices

grtcdr merged commit 1db4c78 into Macchina-CLI:main Mar 20, 2023

Carterpersall added a commit to Carterpersall/libmacchina that referenced this pull request Mar 20, 2023

Make compatible with Macchina-CLI#140

87b31e9

Rolv-Apneseth mentioned this pull request Mar 25, 2023

Add gpu readout Macchina-CLI/macchina#278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add functionality to list connected gpu(s) #140

Add functionality to list connected gpu(s) #140

Rolv-Apneseth commented Mar 15, 2023 •

edited

Loading

grtcdr Mar 15, 2023

Rolv-Apneseth Mar 15, 2023

grtcdr Mar 16, 2023

Rolv-Apneseth Mar 16, 2023

grtcdr Mar 16, 2023 •

edited

Loading

Rolv-Apneseth Mar 16, 2023

grtcdr Mar 17, 2023

Rolv-Apneseth Mar 17, 2023

grtcdr Mar 17, 2023

Rolv-Apneseth Mar 17, 2023

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 17, 2023

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 17, 2023 •

edited

Loading

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 17, 2023

Rolv-Apneseth commented Mar 17, 2023 •

edited

Loading

grtcdr commented Mar 17, 2023 •

edited

Loading

Rolv-Apneseth commented Mar 17, 2023 •

edited

Loading

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 19, 2023

Rolv-Apneseth commented Mar 19, 2023

grtcdr commented Mar 19, 2023

Rolv-Apneseth commented Mar 19, 2023

grtcdr commented Mar 20, 2023

grtcdr commented Mar 20, 2023

Rolv-Apneseth commented Mar 20, 2023 •

edited

Loading

Add functionality to list connected gpu(s) #140

Add functionality to list connected gpu(s) #140

Conversation

Rolv-Apneseth commented Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grtcdr Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 17, 2023

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 17, 2023 • edited Loading

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 17, 2023

Rolv-Apneseth commented Mar 17, 2023 • edited Loading

grtcdr commented Mar 17, 2023 • edited Loading

Rolv-Apneseth commented Mar 17, 2023 • edited Loading

Rolv-Apneseth commented Mar 17, 2023

grtcdr commented Mar 19, 2023

Rolv-Apneseth commented Mar 19, 2023

grtcdr commented Mar 19, 2023

Rolv-Apneseth commented Mar 19, 2023

grtcdr commented Mar 20, 2023

grtcdr commented Mar 20, 2023

Rolv-Apneseth commented Mar 20, 2023 • edited Loading

Rolv-Apneseth commented Mar 15, 2023 •

edited

Loading

grtcdr Mar 16, 2023 •

edited

Loading

grtcdr commented Mar 17, 2023 •

edited

Loading

Rolv-Apneseth commented Mar 17, 2023 •

edited

Loading

grtcdr commented Mar 17, 2023 •

edited

Loading

Rolv-Apneseth commented Mar 17, 2023 •

edited

Loading

Rolv-Apneseth commented Mar 20, 2023 •

edited

Loading