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 the calculate_rs function to slurmutils #42

Merged

Conversation

NucciTheBoss
Copy link
Member

This PR adds @dsloanm's mega-based calculate_rs method to slurmutils so that it can be easily consumed by other charms. I copied it into the utils module, but it can be imported through the top-level module:

from slurmutils import calculate_rs

I also added some unit tests and documentation for calculate_rs so that we have some examples of how it can be used to calculate device file and node name ranges.

@NucciTheBoss NucciTheBoss added the Type: Enhancement Proposes a new feature to be added to the project. label Jan 8, 2025
@NucciTheBoss NucciTheBoss requested review from dsloanm, jedel1043 and a team January 8, 2025 22:51
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! Just a minor suggestion for the README.

README.md Outdated Show resolved Hide resolved
`calculate_rs` can only calculate the ranges and strides on an
iterable which contains integers. The function uses subtraction
to calculate the ranges which is not a valid operation on strings
even though the built-in `sorted` function can sort integers typed
as strings (e.g "1", "2", "3", etc.) in ascending order.

Signed-off-by: Jason C. Nucciarone <[email protected]>
`calculate_rs` must mention that elements should be unique,
otherwise the ranges and strides calculation will fail.

Signed-off-by: Jason C. Nucciarone <[email protected]>
@NucciTheBoss NucciTheBoss force-pushed the nuccitheboss/feat/add-calculate-rs branch from c66dfb6 to 2c1abc3 Compare January 9, 2025 14:01
@NucciTheBoss NucciTheBoss merged commit 499701d into charmed-hpc:main Jan 9, 2025
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