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

rfc14: clarify resource range keys restrictions and/or default behaviour #444

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

sam-maloney
Copy link
Contributor

Problem: the acceptable values for the keys within a resource range dictionary are not entirely clear from the text, and some behaviour differs between flux-core and flux-sched

Firstly, once I started testing some resource ranges, it's clear that having an operand of 1 for either the '' or '^' operators is nonsensical, since min1 or min^1 don't change anything. Also for the '^' operator having min==1 is nonsensical, since 1^operand also goes nowhere. However, once I got looking at the implementation in flux-sched, I noticed that if an operand of 1 is given with either the '*' or '^' operators, there is an if statement which simply short circuits to return the min value. I personally think this is non-intuitive behaviour; it feels to me like such nonsensical input is likely an unintentional mistake from the user that should be failed with a useful error message to have the user clarify what they actually intended, rather than quietly using the min and potentially hiding the mistake from them.

Seondly, the flux-core validator and the jsonschema in RFC 14 both require that either none or all of max/operator/operand keys MUST be specified; however, flux-sched doesn't... because RFC 14 declares that "The default value for max SHALL be infinite, therefore a count which specifies only the min key SHALL be considered a request for at least that number of a resource", this implies a default operator of '+' with a default operand of 1, which is how flux-sched implements this. I personally think either choice is reasonable (require all three, or use defaults), but obviously it should be consistent between the various flux components, and fully documented either way.

This pull request currently updates the RFC 14 text to require all three keys be co-dependent, and clearly defines the limits on operator/operand combinations, because that is what I had written up before realizing that flux-sched had some things implemented differently. But I'm happy to write an update instead specifying the default behaviours of min/max/operand if that route is preferred! I'm also happy to try and implement some of the actual changes in core/sched once a consensus is reached, but figured a discussion here was needed first on the preferred interpretation :)

Problem: the acceptable values for the keys within a resource range
dictionary are not entirely clear from the text

Expand the definitions to clearly explain acceptable types and values,
including dependencies between the keys
@grondo
Copy link
Contributor

grondo commented Feb 13, 2025

This looks like a very good clarification to me.

Requiring all 3 keys seems reasonable. Though our current set of cli submission commands in flux-core do not support this feature, if and when they do they can easily be written to fill in the required fields, so there is not much of an argument for the defaults IMO (i.e. users are not required to create jobspec by hand).

However, since the shorthand is currently used in Fluxion, which has many research thrusts which use Jobspec features beyond v1 (unlike flux-core), @trws or another Fluxion developer should have a look for final approval.

@trws
Copy link
Member

trws commented Feb 13, 2025

I agree, this looks good to me. If we decide to also allow an idset as a count, we'll need to adjust it, but I would expect that to be mutually exclusive with these anyway so they're pretty orthogonal. Thanks for the contribution @sam-maloney!

Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

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

Since @trws approved I'll set MWP here. Thanks @sam-maloney!

@mergify mergify bot merged commit a1668ce into flux-framework:master Feb 13, 2025
7 checks passed
@sam-maloney sam-maloney deleted the rfc14-operator-operand branch February 14, 2025 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants