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

[Bug]: SlurmConfig.from_dict not verifying correct shape of dict-like and array-like configs #17

Open
jedel1043 opened this issue Aug 28, 2024 · 3 comments
Labels
Status: Triage Analysis of issue needs to completed before work can be undertaken. Type: Bug Issue reports a bug, or pull request fixes a bug.

Comments

@jedel1043
Copy link
Contributor

jedel1043 commented Aug 28, 2024

Bug Description

The SlurmConfig.from_dict method doesn't verify that the input fields for dict-like and array-like configs are of the correct shape; Nodes, DownNodes, Partitions, etc.

To Reproduce

config = SlurmConfig.from_dict({
    "Nodes": [
        {
            "NodeAddr": "10.152.28.98",
            "CPUs": "9",
            "RealMemory": "9000",
            "TmpDisk": "90000",
        },
    ],
    "DownNodes": {
        "DownNodes": ["juju-c9fc6f-6", "juju-c9fc6f-7"],
        "State": "DOWN",
        "Reason": "New nodes",
    },
    "Partitions": [
        {
            "MaxTime": "10",
            "MaxNodes": "5",
            "State": "UP",
        },
    ],
})

print(config.nodes) // Prints the malformed "Nodes" array
print(config.down_nodes) // Prints the malformed "DownNodes" object
print(config.partitions) // Prints the malformed "Partitions" array

Environment

Ran locally on Ubuntu Noble (24.04)

> python3 -m pip --version                                                                      
pip 24.0 from *** (python 3.12)

Additional context

This operation should probably throw when the input configs are not of the correct shape.

@jedel1043 jedel1043 added Status: Triage Analysis of issue needs to completed before work can be undertaken. Type: Bug Issue reports a bug, or pull request fixes a bug. labels Aug 28, 2024
@NucciTheBoss
Copy link
Member

Yeah, this is kinda expected. The from_dict(...) constructor is really no thrills for SlurmConfig. It just checks against the SlurmConfigOptionSet to ensure that there are no unexpected configuration keys passed to __init__.

This should be a milestone for a future slurmutils. Ideally we can add this form checking back in when we refine the story around the NodeMap, PartitionMap, etc data structures that can help verify is the data is good or not.

@NucciTheBoss NucciTheBoss changed the title SlurmConfig.from_dict not verifying correct shape of dict-like and array-like configs [Bug]: SlurmConfig.from_dict not verifying correct shape of dict-like and array-like configs Sep 20, 2024
@daniell-olaitan
Copy link
Contributor

daniell-olaitan commented Sep 21, 2024

I believe adding a temporary fix to validate the shape of dict-like and array-like fields (e.g., Nodes, DownNodes, Partitions) could significantly improve the from_dict method's robustness. While I understand that the current implementation primarily checks for unexpected keys, adding basic structural validation could prevent malformed data from being passed through and causing subtle bugs.

A simple check for fields like Nodes (ensuring it's a list of dicts, for example) would help catch configuration errors early, without requiring a major overhaul. This would serve as an interim solution until the more refined NodeMap and PartitionMap data structures are implemented.

I’d love to contribute to implementing this temporary fix if the team agrees it’s a helpful step forward.

@NucciTheBoss

@NucciTheBoss
Copy link
Member

Hi there @daniell-olaitan 👋

Sure, if you'd like to take a crack at a temporary fix, we can help by reviewing your PR 😄

Ideally in the future I'd like to get it where there's proper linting of the entire Slurm configuration data via callbacks, but for now we can include checks to ensure that the data structs are (a) the proper type and (b) "well-formed" to reduce the chances of semantic errors occurring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage Analysis of issue needs to completed before work can be undertaken. Type: Bug Issue reports a bug, or pull request fixes a bug.
Projects
None yet
Development

No branches or pull requests

3 participants