add more arguments to Python JobspecV1 factory methods
#6858
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if queue is not None: | ||
| self.setattr("queue", queue) | ||
| if bank is not None: | ||
| self.setattr("bank", bank) |
There was a problem hiding this comment.
Since you're adding some flux-accounting attributes, you could consider adding project (the flux-accounting implementation of "WCKeys") as another option here too, but not a huge deal either way; I'm not sure how much use projects get these days. I can't remember if it has to be an official jobspec v1 property to be listed here, though, so if so, feel free to ignore me.
There was a problem hiding this comment.
I think we were just trying to add options that are supported in the submission CLI commands 🤷. Since the submission commands support --bank and --queue, I've added those here. However, I did leave other stuff out somewhat arbitrarily so I'm open to other ideas.
There was a problem hiding this comment.
Ah I see. I'm sorry about that! In that case, you can ignore this suggestion 😅
There was a problem hiding this comment.
NP! It was a good point to bring up.
| "none", | ||
| ) | ||
| self.assertEqual(jobspec.getattr("system.queue"), "testq") | ||
| self.assertEqual(jobspec.getattr("system.bank"), "testbank") | ||
|
|
||
| def test_23_from_nest_command(self): | ||
| """Test that `from_batch_command` produces a valid jobspec""" |
There was a problem hiding this comment.
Next door I realize but I assume this should be from_nest_command?
| """Test that `from_batch_command` produces a valid jobspec""" | |
| """Test that `from_nest_command` produces a valid jobspec""" |
There was a problem hiding this comment.
Good catch! I'll add a fix for that.
| if unbuffered: | ||
| # For output, set the buffer.type to none and reduce the configured | ||
| # event batch-timeout to something very small. | ||
| self.setattr_shell_option("output.stdout.buffer.type", "none") | ||
| self.setattr_shell_option("output.stderr.buffer.type", "none") | ||
| self.setattr_shell_option("output.batch-timeout", 0.05) |
There was a problem hiding this comment.
This one seems to be the odd one out, in that it can be set with a keyword argument but not with a property is that right? I think all of the others are both at this point.
There was a problem hiding this comment.
Yeah, we never did add a setter/getter for that one. Do you feel we should add that?
Oh, the other thing I did change what that the factory function arguments for file output are stdout, stderr, stdin, whereas the command line arguments are output, error, and input. I wonder if I should tweak them to match.
There was a problem hiding this comment.
I was waffling on it when I asked, but having given it some thought I think yes. That way both modes work, and we could handle all they keyword args somewhat uniformly with a version of if <name> is not None: self.name = name since the logic can live in the property. It's not a strong preference, but feels more regular to me somehow.
On the args, that's a good point. The difference isn't ideal, I'm thinking maybe we add aliases that are the command line names, then if we want to squash the std* ones we could mark them deprecated for a while at some point.
|
I really like this, had one tiny docstring tweak and a question but it looks really good @grondo. |
|
Hey @grondo, thanks for this improvement! The original request from Ramesh was for explicit documentation on what could be set in the 
It looks like I need to make the following modifications:
Is that correct? |
Problem: The Jobspec stdio property getters and setters have names that do not match the CLI options, which may be not be ideal. Rename the Jobspec stdio properties from `stdin`, `stdout`, `stderr`, to `input`, `output`, and `error` to match the respective cli options. Keep the existing properties as aliases for the new names.
Problem: The Jobspec stdio attributes are tested using the `stdout`, `stderr`, and `stdin` properties, but the preferred names are now `output`, `error` and `input` to match the cli option names. Add unit tests for the new property names.
Yeah, this should be I'm about to open a PR to make
Yes, though none of this work has been merged into a flux-core release, so you might just mention "More parameters will be added to the jobspec initializers in a future Flux release. |
Problem: The Jobspec getattr() and setattr() methods to get and set jobspec attributes do not behave similarly. While both methods default to an `attributes.` prefix if not provided, setattr() further defaults to an `attributes.system.` prefix if the prefix is not `user.` or `system.`, while getattr() does not. Unify the behavior of these two function so they both default to `attributes.system.`. Abstract the prepend of the default key path to a helper function to reduce duplicate code.
Problem: The jobspec getattr tests in t0010-job.py make a note that
setattr and getattr default behavior differ, but this is no longer
true.
Remove the stale comment. Update the tests to ensure that
jobspec.getattr("key")
works in place of
jobspec.getattr("attributes.system.key")
Problem: There is no Python helper function to delete a dotted key in a dictionary similar to how `set_treedict()` sets a dotted key. Add `flux.util.del_treedict`, which works like `set_treedict()`, but deletes the dotted key, e.g. del_treedict(target, "a.b.c")
Problem: There are no unit tests for `del_treedict`. Add some tests to t/python/t0024-util.py.
Problem: There is no unbuffered getter and setter for the `Jobspec` Python class. Add it.
Problem: The cli commands set various attributes in Jobspec manually when the --unbuffered option is used, but there's a property setter for that now. Use the jobspec.unbuffered property setter instead.
Problem: There are no tests of the Jobspec `unbuffered` property getter and setter. Add tests.
Problem: Common jobspec attributes and options are not available in the JobspecV1 factory methods, `per_resource`, `from_command`, `from_batch_command` and `from_nest_command`. This unnecessarily requires the user to set some critical attributes of the resulting jobspec after it is returned, for example the working directory and environment, which makes the interface awkward and error prone. Add a set of the likely most commonly used extra parameters to `JobspecV1.from_command()`. Pass these args along in `**kwargs` from the other factory methods so the extra parameters are supported there as well. Update `from_command()` docstring to the Google style and document the new args. Document `**kwargs` in other functions and reference the docs in `from_command()`.
Problem: There is code in the Python cli submission cli classes that is duplicated by the new arguments available in JobspecV1 factory methods. Where possible, pass arguments to JobspecV1 factory methods in the cli classes when constructing jobspec instead of setting attributes after the fact. Drop the duplicate code. Where the cli commands offer special functionality not directly supported in the JobspecV1 factory methods (such as for the jobspec environment), keep the existing code that sets attributes after jobspec is created.
Problem: The comment for `test_23_from_nest_command` in t0010-job.py incorrectly mentions `from_batch_command`. Fix the comment.
Problem: The new arguments for JobspecV1 factory functions do not have any tests. Add a few tests to python/t0010-job.py.
Problem: CodeQL warns that empty except clauses should include a comment, but several except: pass clauses in flux.cli.base do not do so. Add comments to empty except clauses in cli/base.py.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6858 +/- ##
==========================================
- Coverage 83.85% 83.85% -0.01%
==========================================
Files 538 538
Lines 89912 89966 +54
==========================================
+ Hits 75399 75438 +39
- Misses 14513 14528 +15
🚀 New features to boost your workflow:
|
This PR adds some more arguments to the
JobspecV1factory methods as described in #6833 so that more fully formed jobspec can be instantiated with a single method call.This PR doesn't go as far as adding all the options offered by the submission CLI commands. However, job parameters that are an annoyance to set after the fact like the environment and cwd are there, as well as output, error, and input paths, etc.
One thing I didn't do was move support for some of the more user-friendly aspects of the CLI into the Python methods. For instance, the
environmentandrlimitsarguments just take straight mappings, instead the "rule" based expressions supported by the CLI. We could change that, but I wasn't sure if it was necessary in the Python API.Also, some other missing parameters include
--dependency,--requires, and--begin-time-- all of which actually are mostly implemented in the CLI, and I wasn't sure exactly if those should go in the factory methods. They could be added later I guess.Fixes #6833