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 support of nvidia graphics using nvidia-smi #3242

Closed
wants to merge 3 commits into from

Conversation

voidThread
Copy link

Hi!
I've introduced the new module which pulls data from the external 'nvidia-smi' program.
The module runs 'nvidia-smi' as a forked process and saves GPU monitor data into the exchange file. If not specified in the configuration file, the file is created in the system's temporary directory. You can define your own exchange file, but you must ensure that it exists and has proper privileges set.
Since this is my first contribution here, I'm eager to receive feedback on any areas that could be improved.

@haug1
Copy link
Contributor

haug1 commented May 9, 2024

I'm not one to comment on this really and I can't test it unfortunately, but from a quick skim of the code and the assumption that you've tested it too work on your machine; great work, looks like you've done a really good job with this.

I can give you a heads-up about the man pages, it's probably going to get requested that you add some documentation for how to use this if it is to get merged.

Some kind of conditional include in the build step, like what's done with the cava module (if you search "HAVE_LIBCAVA" and follow the trail, you can try to do something similar) would also probably be nice. I'm not sure what's possible, should probably have lead with I'm pretty bad at C++.

@voidThread
Copy link
Author

Thanks, @haug1 !
I've been using this for a few days without any problems.
Also, I've added what you proposed.

@haug1
Copy link
Contributor

haug1 commented May 10, 2024

I didn't think of this yesterday, sorry. But this might be simply and more correctly achieved for users with a custom module instead so to manage the scope of the project and therefore not interesting to merge. Again, I'm not really one to comment on this. Just thinking out loud. :)

@Alexays
Copy link
Owner

Alexays commented May 28, 2024

Yup, like @haug1 said, theses specific modules, as pretty simple should be done in a custom module :)

@Alexays Alexays closed this May 28, 2024
@haug1
Copy link
Contributor

haug1 commented May 28, 2024

Sorry to have added work on you with my first comment @voidThread, that was not my intention. Just wanted to give some feedback, but didn't think it through until later.

(Also, sorry to that other guy I accidentally tagged.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants