Improve range validation and cleanup validator

[sam-maloney]

This PR adds more thorough checks for min/max/operand/operator combinations for resource count complex ranges, accompanied by new/modified "invalid" jobspec examples to match the validation and increase cover. The penultimate commit clarifies some "typos" in error messages, and the final commit simplifies the validation of required top-level keys using positional arguments in the class constructors (feel free to revert this if you disagree, it just felt much cleaner when I was examining how everything was implemented).

One point that merits attention is that I have made the max key always optional in the complex range; this disagrees with with the current RFC14 (and my earlier change in flux-framework/rfc#444) but after implementing the count infrastructure and the range string spec in RFC45 it seems clear to me that the max key should always be optional to indicate an unbounded/infinite/give_me_everything_you_got request, without the user or CLI tools having to otherwise insert an arbitrary large number whenever a non-default operand/operator combination is used. If this seems sensible to others I am of course happy to tweak RFC14 to match (and I suppose also update my changes from flux-framework/flux-sched#1341)

[grondo]

Thanks @sam-maloney! These changes look great! An optional max parameter for complex ranges makes a lot of sense to me. However, I would want @trws or @milroy to comment about Fluxion behavior before merging.

How do you envision this looking on the command line, i.e. the difference between specifying an exact count vs a minimum with unlimited maximum. (Sorry if you've already explained this elsewhere, I probably lost track of any discussion here)

[sam-maloney]

How do you envision this looking on the command line, i.e. the difference between specifying an exact count vs a minimum with unlimited maximum. (Sorry if you've already explained this elsewhere, I probably lost track of any discussion here)

@grondo My initial thought would be simply allowing RFC45 strings to be used for current CLI options, e.g.

flux run -n 1+ hostname

or

flux run -n 2+:2:* hostname

where in the second example it's implicit in RFC45 that the max is optional even when non-default operator/operand combinations are used, so that a user never needs to write something like flux run -N 2-RANGE_MAX:2:* hostname which would require knowledge (or guesswork) as to what RANGE_MAX should be, or what the full system size is.

Eventually a CLI option/plugin to allow use of RFC46 strings would also be nice for testing or power users, something like

flux run --resources="slot=2+:2:*/core" hostname

[sam-maloney]

However, I would want @trws or @milroy to comment about Fluxion behavior before merging.

FWIW, Fluxion originally used defaults for all three of max/operand/operator and I was the one who added the constraints to match the flux-core validation. So this would simply be a partial revert to separate the requirement for the max key from the other two, while keeping the improved validation of operand/operator combinations. Still good to have their input, but not really anything new, just me understanding better what actually makes sense, haha!

[trws]

That sounds right. The fluxion code that reads from these always uses the max currently though, and somewhat relies on there being an upper bound. We can certainly make the max optional, but the effect will be that the min will become the max until we have time to work through how to handle unbounded requests.

[grondo]

Looks good to me on the flux-core side! Thanks @sam-maloney and @trws!

[grondo]

Setting MWP here. Thanks again @sam-maloney

[mergify]

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks.

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Approving reviews have been dismissed because this pull request
was updated.

[grondo]

@Mergifyio requeue

[mergify]

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

[codecov]

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.86%. Comparing base (7a81d53) to head (6b136a9).
⚠️ Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/bindings/python/flux/job/Jobspec.py	96.77%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7130   +/-   ##
=======================================
  Coverage   83.85%   83.86%           
=======================================
  Files         551      551           
  Lines       93303    93304    +1     
=======================================
+ Hits        78241    78250    +9     
+ Misses      15062    15054    -8     

Files with missing lines	Coverage Δ
src/bindings/python/flux/job/Jobspec.py	92.95% <96.77%> (+1.18%)	⬆️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.