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

feat: add gres.conf editor to slurmutils #37

Merged

Conversation

NucciTheBoss
Copy link
Member

This PR adds the gresconfig module for editing gres.conf configuration files on machines. This module is necessary for configuring GPUs in Charmed HPC.

Key things to note

  • This editor uses a new approach for defining descriptors for mutating configuration options through data models. Rather than dynamically create descriptors using the generate_descriptors(...) function, they're instead manually defined. Static type checkers such as pyright do not effectively support dynamic attributes without ugly hacks, so here I just created all the descriptors using live templates in PyCharm. Ctrl+J propsd will quickly enter the boilerplate for the descriptors.

    This should make it easier for using a static type checker on code that uses slurmutils without needing to deactivate checks. If we're happy with how it looks here, I will look into applying it to all the other data models within slurmutils.

  • Why are all the deleters encapsulated with try: ... except AttributeError: pass ? I realized that if you call del on a data model attribute before setting a key within the internal dictionary associated with the created data model, Python will raise an AttributeError since the key does exist inside the internal dictionary. Since the desired behavior is that the configuration option is unset and not present in the generated configuration file, the AttributeError is silently handled rather than emit a warning message.

Changes:

* Adds `GRESConfig` for modelling the gres.conf configuration file.
* Adds `GRESName` for modelling lines in the gres.conf configuration file.
* Adds `GRESNode` for modelling nodes with configured generic resources in gres.conf.

Signed-off-by: Jason C. Nucciarone <[email protected]>
@NucciTheBoss NucciTheBoss added the Type: Enhancement Proposes a new feature to be added to the project. label Dec 10, 2024
Release adds new `gres.conf` editor. Also tests out explicitly
defining descriptors for static typing tools such as `pyright` or `mypy`

Signed-off-by: Jason C. Nucciarone <[email protected]>
@NucciTheBoss NucciTheBoss force-pushed the nuccitheboss/feat/gres-editor branch from 3cb0a89 to 0f20a0a Compare December 10, 2024 22:29
Copy link

@dsloanm dsloanm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I checked out the branch and created a few test gres.confs without issue. I see GresTypes and Gres have been in slurm.conf for a while so no changes needed there.

For the getters/setters/deleters, my preference would be for dynamic over IDE-generated boilerplate, just to make the code more concise, but I can see your point re: pyright.

@NucciTheBoss
Copy link
Member Author

For the getters/setters/deleters, my preference would be for dynamic over IDE-generated boilerplate, just to make the code more concise, but I can see your point re: pyright.

That's a fair point from the library development perspective; you can only do Ctrl+J propsd so many times before it starts to wear on you, however, I do think that there's trade-offs to both approaches.

Dynamic attributes save you time when developing slurmutils data model objects, but it makes it quite difficult to fully leverage the benefits of static type checkers in downstream projects. For example, pyright "explodes" in the Slurm charms unless we disable reporting missing attributes. We also lose the benefit of tab-completion. While technically it doesn't change anything in the downstream code, it still is pretty nice from the UX perspective that entering something like config. will bring up all the available configuration options.

Explicit attributes take more time to create (and possibly maintain), but it allows us to be serious about static type checking. It's also arguably better to be explicit rather than implicit as there's less ambiguity about what data models can and can't do.

@NucciTheBoss NucciTheBoss merged commit 60bc8e6 into charmed-hpc:main Dec 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Proposes a new feature to be added to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants