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

python: allow resource count of 0 in Jobspec v1 #4259

Merged
merged 6 commits into from
Apr 3, 2022

Conversation

grondo
Copy link
Contributor

@grondo grondo commented Apr 2, 2022

When going through old issues I noticed #3275, which seemed like a reasonable and easy fix.

This change allows a count of 0 for core and gpu resources (not sure if core is all that useful, but this addresses the main point of the issue)

@grondo grondo added this to the flux-core v0.38.0 milestone Apr 2, 2022
Problem: The yaml jobspec invalid/resource_slot_with_zero.yaml
actually sets the value of the 'core' resource count to zero, not
the slot resource.

Move the 0 count to the 'slot' definition instead of 'core'.
Problem: Jobspec v1 doesn't require that resources have a count greater
than zero, but the Jobspec Python class raises an exception if this
is so.  This is inconvenient for users that want to do things like
jobspec templating, since the resource entry must be removed instead
of simply zeroed.

Allow a count of 0 for resources other than "node", "slot", or "core",
since zero for these specific resource types doesn't (currently) make
any sense.

Fixes flux-framework#3275
@grondo
Copy link
Contributor Author

grondo commented Apr 2, 2022

I force-pushed an update that reduces the changes here:

  • dropped the change to allow "complex" resource count specification (min, max) to be zero (this was probably gratuitous)
  • restrict the allowable resource types that can have a count of 0 to anything that is not node, slot or core

Also added a test to ensure a gpu count of 0 passes validation.

Problem: The Python jobspec v1 class allows a count of 0 for
resources that aren't node, slot or core, but the jobspec1 C API
rejects a jobspec with any resource count of 0 as invalid.

Allow gpu resources specifically to have a count of 0 since this
is the only resource supported by the jobspec1 class besides node,
slot and core.
Problem: The Python and libjob jobspec1 implementations both
have been relaxed to allow a count of 0 for some resource types,
but the schemas in src/modules/job-ingest/schemas fail to validate
newly allowed jobspec.

Update the jobspec jsonschema to allow a count 1 of resources other
than "node", "slot", and "core" to match the other jobspec validators
in flux-core.
Problem: Detection of an invalid core count in jobspec is not currently
exercised in jobspec validation testing.

Add a jobspec with a 0 core count to t/jobspec/invalid_v1.
Problem: No test verifies that a v1 jobspec with a gpu count of 0
is accepted as valid, but a gpu count of -1 is rejected as invalid.

Add a jobspec with 0 gpu count to t/jobspec/valid_v1, and a jobspec
with a -1 gpu count to t/jobspec/invalid_v1.
@codecov
Copy link

codecov bot commented Apr 3, 2022

Codecov Report

Merging #4259 (3bea591) into master (31d5a26) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##           master    #4259      +/-   ##
==========================================
- Coverage   83.55%   83.55%   -0.01%     
==========================================
  Files         388      388              
  Lines       64993    64998       +5     
==========================================
+ Hits        54307    54310       +3     
- Misses      10686    10688       +2     
Impacted Files Coverage Δ
src/bindings/python/flux/job/Jobspec.py 83.07% <75.00%> (-0.18%) ⬇️
src/common/libjob/jobspec1.c 84.54% <100.00%> (+0.10%) ⬆️
src/cmd/top/summary_pane.c 83.41% <0.00%> (-2.52%) ⬇️
src/common/libterminus/terminus.c 85.82% <0.00%> (-0.25%) ⬇️
src/broker/overlay.c 86.82% <0.00%> (+0.10%) ⬆️
src/broker/state_machine.c 82.03% <0.00%> (+0.19%) ⬆️
src/modules/job-info/guest_watch.c 77.56% <0.00%> (+0.27%) ⬆️
src/common/libsubprocess/subprocess.c 89.00% <0.00%> (+0.30%) ⬆️

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

LGTM!

@grondo
Copy link
Contributor Author

grondo commented Apr 3, 2022

Thanks! Setting MWP.

@mergify mergify bot merged commit 936b9ed into flux-framework:master Apr 3, 2022
@grondo grondo deleted the issue#3275 branch April 3, 2022 15:23
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.

2 participants