add CLIPlugin to accept arbitrary R shapes/JSON on the command-line #7252
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
2 times, most recently
from
25f1bbd to
bd2e57b
Compare
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
from
bd2e57b to
a432a63
Compare
|
@sam-maloney was kind enough to do a quick review on 1/8/26 and commented in Slack:
|
grondo
left a comment
grondo
left a comment
There was a problem hiding this comment.
Nice work! Just a few comments or questions to address inline. Thanks!
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
from
a432a63 to
3d3172e
Compare
There was a problem hiding this comment.
It is getting to the point where @sam-maloney's --resource-shape option will be really useful so I made another pass on this one.
I'm also wondering if we should install the plugin so it is picked up by default, instead of requiring users to modify their plugin path to get the feature.
|
This would indeed be handy for toy scheduler experiments. Ping? |
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
from
3d3172e to
db89f76
Compare
|
... I have no idea why this closed. Edit: p.s. this is not ready for another review, a few more modifications needed, but will be soon. |
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
4 times, most recently
from
6e90be0 to
501c14e
Compare
|
This is ready for another review. I would like to get an ack from @sam-maloney before we merge, particularly on the testing and 501c14e, since it modifies his contribution and was Claude-generated. On balance, it looks reasonable to me, but Sam is the expert on the parser. If he's good with that, I will fold it up into the initial commit. As for
that will depend on #7244, which I plan to go work on now. |
|
I was able to reproduce the el8 issue locally: Generating LALR tables
Traceback (most recent call last):
File "/usr/lib64/python3.6/runpy.py", line 193, in _run_module_as_main
"__main__", mod_spec)
File "/usr/lib64/python3.6/runpy.py", line 85, in _run_code
exec(code, run_globals)
File "/usr/src/src/bindings/python/flux/shape/parser.py", line 666, in <module>
print(yaml.dump(result, sort_keys=False))
File "/usr/lib64/python3.6/site-packages/yaml/__init__.py", line 200, in dump
return dump_all([data], stream, Dumper=Dumper, **kwds)
TypeError: dump_all() got an unexpected keyword argument 'sort_keys'
not ok 13 - flux.shape.parser --yaml produces yaml output
#
# cat >expected10 <<-EOF &&
# - type: slot
# count: 4
# label: task
# with:
# - type: node
# count: 1
#
# EOF
# $parser --yaml -- slot=4/node >output10 &&
# test_cmp expected10 output10
#According to the pyyaml changelog, support was added for the I tried removing that key, but it does change the output: --- expected10 2026-03-31 15:20:23.384955001 +0000
+++ output10 2026-03-31 15:20:23.489955001 +0000
@@ -1,7 +1,6 @@
-- type: slot
- count: 4
+- count: 4
label: task
+ type: slot
with:
- - type: node
- count: 1
+ - {count: 1, type: node}Perhaps that's not so bad, and would most likely be consistent across all distros? We could always add the key back in one glorious day when we finally no longer are tied to Python 3.6.8... |
|
We could vendor pyyaml-5.1, or build it ourselves with pip. It appears to support python 3.6 according to the same changelog. However, if we do something to the el8 container, we'll have to remember and find a similar solution for all the TOSS 4 systems. |
|
I don't think we'd want to vendor a python package just for testing purposes. Perhaps there is a workaround for sorting the output without |
|
@sam-maloney, barring the one outstanding test failure, would you mind taking a whirl through this and giving your thoughts? :) |
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
2 times, most recently
from
abc190d to
c4d884a
Compare
|
@grondo, do you want to take another look at this for a secondary approval/change request? |
|
|
||
| . $(dirname $0)/sharness.sh | ||
|
|
||
| test_under_flux 4 job |
There was a problem hiding this comment.
Do these tests require a 4 node instance. If they do require a Flux instance, could you get away with 1 rank?
There was a problem hiding this comment.
I like keeping the first test around, because I think it's always good to test that there isn't some logic in the plugin that fails if you don't use it, but you're right -- no need for a 4 node instance. Will swap to 1.
|
I noticed one quick thing. Will circle back again later to check this out again. |
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
from
c4d884a to
7987026
Compare
|
@wihobbs - I know you've been tasked with some testing due early next week, but at a lower priority than that, it would be great to get this PR finalized for our next tag. |
|
This is awaiting a review from @grondo who I believe is also juggling some high-priority items, but we agreed the same in our 1-1 this week. I expect we'll make the May release cut off. ;) |
|
Was just taking a look at this. @wihobbs I guess you get the honor of fixing one last merge conflict in |
grondo
left a comment
grondo
left a comment
There was a problem hiding this comment.
LGTM! A couple minor things inline.
Going forward, I wonder if we should remove the default_plugins list and instead just load all plugins found in the flux.cli.plugin namespace, since that's what is done with other plugin loading schemes in Flux and (I think?) with FLUX_CLI_PLUGINPATH.
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
2 times, most recently
from
4c04e93 to
91892d2
Compare
|
Thanks @grondo! Setting MWP. |
Problem: the --list-plugins test asserts that only 5 plugins should be listed with that option. Until recently that was true, but as plugins get added to the `flux.cli.plugins` namespace this number will go up. In general, it seems cumbersome to edit that test each time a new plugin is added to that namespace. Amend the logic to check for just one known plugin in the output.
Problem: there is a test in python/t0019-cli-plugin.py that checks that if two plugins that have conflicting options are loaded, the first one in the FLUX_CLI_PLUGINPATH* wins. Or so it seemed! All this test was really doing was checking that the registry didn't load both plugins, and never actually checked that the right plugin was loaded. Edit the test to fix this oversight.
Problem: There is no parser for the CLI Jobspec Resource String Syntax proposed in RFC 46. Add a flux.shape.parser Python module which implements a parser using the 'ply' module (mimicking the constraint parser). For testing, the parser may be run from the command line with flux python -m flux.shape.parser [OPTIONS] -- EXPRESSION
Problem: currently, users have no way on the CLI of utilizing the experimental parser in RFC 46 for specifying a job shape or a resource set they'd like to use via JSON. This causes users to defer to cumbersome piping and JSON mangling when they want to submit jobs of arbitrary resource shapes on the command line. Add long options via plugin `--resources-json` & `--resources-shape` for specifying resources on the command line beyond the standard ones. Provide this plugin by default; package it with flux-core into `flux.cli.plugins` and add it to the `default_plugins` in src/bindings/python/flux/cli/plugin.py.
Problem: there are examples of valid job shapes listed in RFC 46 but there is currently no unit test for the python jobspec shape parser. Add a unit test with the examples in RFC 46.
Problem: the jobspec shape/JSON plugin is currently not tested on the command line. Add a sharness test.
Problem: the RFC 46 resource shape parser is tested via its corresponding CLIPlugin and with Python unit tests, but not as a standalone tool. Add t0039-shape-parser.t to fill the testing gap. Assisted-By: Claude:claude-sonnet-4.5
wihobbs
force-pushed
the
cli-jobspec-resources-parser
branch
from
91892d2 to
3c823cc
Compare
Spoke too soon. There was yet another conflict. In the same file, on the same line. |
Merge Queue Status
This pull request spent 15 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7252 +/- ##
==========================================
+ Coverage 83.91% 83.96% +0.04%
==========================================
Files 573 575 +2
Lines 96453 96770 +317
==========================================
+ Hits 80943 81254 +311
- Misses 15510 15516 +6
🚀 New features to boost your workflow:
|
|
Yay! |
5 participants
This PR takes the excellent work done by @sam-maloney for RFC#46 and extends it into a command-line plugin that users can opt in to by setting
FLUX_CLI_PLUGINPATH. This was originally suggested by @grondo as a way for us to test/get user feedback on the ergonomics of specifying shape on the command line, and make progress toward incorporating that option into our standard command line.The plugin is relatively straightforward and the testing just references examples from the corresponding RFC. I know this is coming up right before many 🌴s, so no rush on reviews, but I wanted to get it to a stopping point before my own holiday begins :).