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

Config #151

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Config #151

wants to merge 3 commits into from

Conversation

jenuk
Copy link

@jenuk jenuk commented Apr 7, 2023

Hello @wookayin !

First of all, thank you so much for this package.

I have implemented a a new function to format the output of gpustat using a python dataclass. The class saves a color for each entry provided by gpustat, names are the same as in the json (except for replacing . with _), as well as having a template to layout the final string. This should address issues #51 and #9. Here are two example outputs using it:

gpustat_box_screen
gpustat_default_screen

The output is determined by three strings that are formatted using str.format, one for the header, the gpu info and the process info. In that environment the corresponding information is provided, and can be used using the standard {name:specifier}. I have added an additional parsing for ... $name:specifier$ ... to input the correct color before and switch back to normal formatting afterward. Also as in your code there are some extra things added like the gpu-name width, the blessed terminal interface and an empty string (to repeat a character width times).

There are some open todos that need to be implemented before this should be pushed, but I wanted to ask for your opinion so far before I continue this.

Remaining todos:

  • Connection configuration file <-> dataclass. This should be really easy to do when using one of the many libraries designed to do exactly that. I would suggest omgeconf if you are fine with adding this as another dependency.
  • Currently it's either using the config or command line options. Reading and adding command line options with OmegaConf is no problem. The real issue here is that the config at the moment requires a preconfigured template, so that needs to be dynamically adjustable if no fixed template is specified. Probably the biggest todo.
  • Colors are static at the moment, but I noticed when writing my code that you highlight certain things based on ratios. I'm not sure how customizable this functionality should be. I was thinking of implementing something like "{field a} {operator} {field b) {comp} {field c}", e.g "power_usage / power_limit > 0.9" or "username == jenuk" as a test? I feel like this would support enough flexibility.
  • Maybe adding an option to color preceding/following characters in the same color as the metrics.
  • Updating the readme to explain all changes.

Do you have any additional feedback/wishes for this? If you are happy with this, I will finish it.

@wookayin wookayin mentioned this pull request Apr 10, 2023
@wookayin
Copy link
Owner

Hello @jenuk, thanks so much for your work and great suggestion! This looks like an amazing contribution. I will be happy to discuss and work with you to make this landed in the near future.

It'd be quite an involved job to come up with a right design on config classes and customizable APIs. I find the use of dataclass (requires python 3.7+ though) useful, which allows users to write their own config at ease. We can decide between declarative APIs (e.g. dataclass config) or imperative APIs (or something like plugins, see #152).

I will take a detailed look later, but here are some first-impression thoughts or questions:

  • Seems already in the TODO, but what is the meaning --config? How are users supposed to specify or write config file (and where)? How to override the PrintConfig class?
  • For dynamic conditions (e.g. for colors), I don't like introducing additional DSLs or syntax that are unnecessary. Python code itself is already a flexible and injectable for configuration.

As a final remark, I was thinking about having a more flexible plugin architecture (#152) which would also allow custom formatting but at a slightly different approach. We could even merge these approaches too.

Thank you again!

@jenuk
Copy link
Author

jenuk commented Apr 10, 2023

A plugin based solution seems viable to me, since the current implementation is only really dependent on the json returned by the query. Though I have to say that I have no experience writing plugins for anything, so I don't know what exactly would need doing.


Seems already in the TODO, but what is the meaning --config? How are users supposed to specify or write config file (and where)? How to override the PrintConfig class?

Right now that was more part of testing, and just activates my print instead of the default one. But my intention was to set it to a path to a config (yaml) file, that would then be loaded to override all given options from PrintConfig. This could then default to something like ~/.config/gpustat/config.yaml. (Here there are two options: setting the default value of argparse to that path or only the const, and the default to something like "" so that gpustat --config activates this printing.) So this would allow something like gpustat --config light_config.yaml or gpustat --config test_config.yaml.

Additionally, with things like OmegaConf there is also the option to automatically parse nested assignment into the config. E.g. putting gpustat font_modifiers.hostname=black to make the hostname visible on a white terminal. So short-term things can easily be changed. (Maybe I should shorten the names a little bit.)


For dynamic conditions (e.g. for colors), I don't like introducing additional DSLs or syntax that are unnecessary. Python code itself is already a flexible and injectable for configuration.

So you are talking about the $name:specifier$ thing, right? This is then parsed into {mod.name}{name:specifier}{t.normal} (the specifier and the colon is optional). To me it seemed valueable to make the config writeable in a short-hand form as [$id$] $name$ ... without having to type [{mod.id}{id}{t.normal}] {mod.name}{name}{t.normal}, and still have all the colors specified used. This was the easiest and least intrusive way I could think of two enable both the powerful default python formatting options as well as having the colors by default
An alternative would be to have the specifiers separate from the template string, similar to the color definition, and then parse all options to string using that specifier and adding color, and only then parse them into the final template.


Regarding dataclasses being python 3.7, there are a couple of options:

  • There is a backported version for python 3.6, but i have never tried it. It is on pypi, people on 3.6 could just install that.
  • The dataclass provides ease of use: options can be left out and have a default (making it easy to generate an initial config), misspellings will be noticed and type-checker may report errors directly in the code. However, all that is not critical functionality, in a scenario without dataclass available, the program could be loaded with less functionality at run time without failing and still providing basically the same functionality. E.g. OmegaConf (sorry for going on and on about that :D) is available for 3.6+ python, and parses yaml files by default into a non-structured form of what's basically a dataclass file.
  • Edit: Keeping config based printing and the default printing, and just ignoring the config printing without dataclass.
  • not supporting python 3.6

@XuehaiPan
Copy link
Contributor

XuehaiPan commented Apr 11, 2023

FYI, Python 3.6 reached its end-of-life schedule 1 year and 3 months ago (23 Dec 2021). Python 3.7 will end its maintenance in 2 months and 2 weeks (27 Jun 2023). The default python3 on Ubuntu 18.04 LTS is Python 3.6 and on Ubuntu 20.04 it's Python 3.8. Ubuntu 18.04 LTS will reach the end of its hardware and maintenance update LTS support window on April 30, 2023 (about three weeks later).

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

Successfully merging this pull request may close these issues.

3 participants