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 from_dict method to construct configuration from dictionary #15

Merged
merged 19 commits into from
Aug 26, 2024

Conversation

NucciTheBoss
Copy link
Member

@NucciTheBoss NucciTheBoss commented Aug 26, 2024

TL;DR - adds the from_dict class method to all the configuration data models to enable creating new model objects from dictionaries. For example, here's how you could create a node using the from_dict method:

new_node = Node.from_dict(
    {
        "juju-c9fc6f-5": {
            "NodeAddr": "10.152.28.51",
            "CPUs": "1",
            "RealMemory": "1000",
            "TmpDisk": "10000",
        }
    }
)

The new option module is then responsible for validating that the values passed to from_dict are correct, add provides the marshallers for when you go to dump the configuration into a file. The option module also removes the "hacky" implementation for callbacks which just uses a MappingProxy embedded into the data model as a protected attribute.

BREAKING CHANGES: I removed the NodeMap, PartitionMap, DownNodesList wrappers as I was not enamored with their implementation. They were rushed. I took them out as I'd like to spend more time refining their implementation so they aren't as hacky/clunky. Let me know if you'd like for me to add them back in. We can like open a follow-on PR that focuses on their implementation, but for now we have enough to unblock charmed-hpc/hpc-libs#22

Side note: I need to update the README and pyproject.toml before merging this PR, but I want to know your thoughts on the "from_dict(...) and back again" implementation before going ahead and handling that.

Closes #14

* Changes:
  * Remove unneccessary sections for rendered config files. Header and sections add unneccessary amount of time to marshalling
  * Don't compose methods using `functools.partial` and instead use decorator to supply functionality we need
  * Simplify parsing methods to more straightforward and less complex

Signed-off-by: Jason C. Nucciarone <[email protected]>
…than partial functions

Signed-off-by: Jason C. Nucciarone <[email protected]>
* Dynamically defines descriptors for accessing configuration options.
  * Less LOC and more maintainable as it is easier to modify callbacks for attributes.
* Adds methods that make it much easier to convert between string, dict, json, etc representations.
* Refactors camelizer to be more efficient and faster.
* Uncomplicates descriptor generation by using only the `generate_descriptors` function.
* Adds `option` module to store valid configuration options and validate inputs to data models.

BREAKING CHANGES: Removes custom data structures for manipulating groups of Nodes, Partitions, etc.

Signed-off-by: Jason C. Nucciarone <[email protected]>
* Use from_dict method to create data models.

Signed-off-by: Jason C. Nucciarone <[email protected]>
* Renovate is no longer used on repository to update dependencies.
  Dependabot is used instead as it does not require an open issue
  on the issue tracker to work.

Signed-off-by: Jason C. Nucciarone <[email protected]>
Copy link
Contributor

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looking good! Just found some typos but implementation-wise looks perfect!

slurmutils/editors/slurmdbdconfig.py Outdated Show resolved Hide resolved
slurmutils/editors/slurmdbdconfig.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Did another pass with some questions and some nitpicks.

slurmutils/editors/editor.py Outdated Show resolved Hide resolved
slurmutils/editors/editor.py Outdated Show resolved Hide resolved
slurmutils/models/model.py Show resolved Hide resolved
slurmutils/models/model.py Outdated Show resolved Hide resolved
slurmutils/models/model.py Show resolved Hide resolved
slurmutils/models/option.py Outdated Show resolved Hide resolved
* Have `clean(...)` return None if line is a comment rather than a separate boolean value.

Signed-off-by: Jason C. Nucciarone <[email protected]>
@NucciTheBoss
Copy link
Member Author

@jedel1043 R4R 🔍

Copy link
Contributor

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looks great!

@NucciTheBoss NucciTheBoss merged commit 6621623 into charmed-hpc:main Aug 26, 2024
3 checks passed
@NucciTheBoss NucciTheBoss added the Type: Enhancement Proposes a new feature to be added to the project. label Aug 26, 2024
@jamesbeedy
Copy link

Wow! @NucciTheBoss , nice work!

@NucciTheBoss NucciTheBoss deleted the feat-add-from-dict branch August 29, 2024 14:16
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.

[Enhancement]: Add support for creating Slurm configuration from dictionary
3 participants