Skip to content

Conversation

@hategan
Copy link
Collaborator

@hategan hategan commented Apr 30, 2025

Some deployments require a memory specification when using nodes in non-exclusive mode. This PR adds that feature.

@hategan hategan requested a review from andre-merzky April 30, 2025 02:08
Copy link
Collaborator

@andre-merzky andre-merzky left a comment

Choose a reason for hiding this comment

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

Thanks Mikhail - that all looks good. The only comment (and I'm not sure it warrants any change) is that kb is a bit arbitrary and small - would any application ever specify memory in that granularity? But I also agree it's better to err on the site of finer granularity than on the coarser end...

Approving!

@hategan
Copy link
Collaborator Author

hategan commented Apr 30, 2025

Thanks Mikhail - that all looks good. The only comment (and I'm not sure it warrants any change) is that kb is a bit arbitrary and small - would any application ever specify memory in that granularity? But I also agree it's better to err on the site of finer granularity than on the coarser end...

Approving!

The resource spec has it in bytes. But some of the schedulers don't use granularity lower than KB (i.e., --mem 1 means 1 KB and --mem 1b is not allowed). The templates explicitly have the unit for clarity.

@andre-merzky
Copy link
Collaborator

Thanks Mikhail - that all looks good. The only comment (and I'm not sure it warrants any change) is that kb is a bit arbitrary and small - would any application ever specify memory in that granularity? But I also agree it's better to err on the site of finer granularity than on the coarser end...
Approving!

The resource spec has it in bytes. But some of the schedulers don't use granularity lower than KB (i.e., --mem 1 means 1 KB and --mem 1b is not allowed). The templates explicitly have the unit for clarity.

Thanks for clarifying - as the smallest common denominator, the unit does make sense.

@hategan hategan merged commit f8b54ff into main May 8, 2025
12 checks passed
@hategan hategan deleted the add_mem branch May 8, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants