DM-28653: Initial changes to support execution butler. #38
Conversation
MichelleGower
force-pushed
the
tickets/DM-28653
branch
from
58472c4
to
2bfff46
Compare
MichelleGower
force-pushed
the
tickets/DM-28653
branch
from
a37b435
to
151998b
Compare
python/lsst/ctrl/bps/bps_config.py
Outdated
| if isinstance(other, str): | ||
| # First load default config from ctrl_bps, then | ||
| # override with user config. | ||
| bps_defaults = realpath(dirname(__file__) + "/../../../../etc/bps_defaults.yaml") |
There was a problem hiding this comment.
Can we use os.path.join() to concatenate the paths instead? It would lower the risk of running into problems due to extra (or missing) backslashes.
There was a problem hiding this comment.
Wasn't happy with the hardcoded relative path. @timj pointed me to pkg_resources and from there I ended up with importlib.resources. To use this, etc/bps_defaults.yaml will need to be moved down inside python/lsst/ctrl/bps.
python/lsst/ctrl/bps/bps_utils.py
Outdated
| """Create a filename to be used when storing the QuantumGraph | ||
| for a job. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| job : `lsst.ctrl.bps.GenericWorkflowJob` |
There was a problem hiding this comment.
It looks like this line should be removed. The parameter job is the second parameter in function's signature and it is properly described later.
| ) | ||
|
|
||
|
|
||
| class SubmissionOptions(OptionGroup): # noqa: N801 |
There was a problem hiding this comment.
You don't need the # noqa comment. Your class name follows CapWords convention so there's nothing here flake8 should complain about.
| "pipeline": "pipelineYaml"} | ||
| for key, value in kwargs.items(): | ||
| # Don't want to override config with None values | ||
| if value: |
There was a problem hiding this comment.
If your only concern is to not override the config values with Nones then
if value is not None:
...feels like be a better option (bonus, no need for the comment 😉). If you're also worried about overriding existing values with empty strings then the comment is not entirely accurate.
There was a problem hiding this comment.
I cannot think of use case at the moment to need command line options to set these particular values as empty strings. Changing comment to include empty string.
|
|
||
| Parameters | ||
| ---------- | ||
| final : `lsst.ctrl.bps.GenericWorkflowJob` or |
There was a problem hiding this comment.
You may want to end this line with \ (line continuation). If I recall correctly, either sphinx or package-docs wasn't entirely happy with a type specification spanning over more than one line when I tried to pull it off in line 456.
| if executable is not None: | ||
| self._executables[executable.name] = executable | ||
| else: | ||
| _LOG.warning("add_executable the executable is None") |
There was a problem hiding this comment.
Can we change the message to something descriptive, for example "executable not specified (None); cannot add to the workflow's list of the executables"?
| """Read a quantum graph from a file or create one from scratch. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config : `lsst.ctrl.bps.BpsConfig` | ||
| Configuration values for BPS. In particular, looking for qgraphFile. | ||
| out_prefix : `str` or None | ||
| out_prefix : `str` |
There was a problem hiding this comment.
The qualifier optional is missing in the type description.
| """Create QuantumGraph from pipeline definition. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| config : `lsst.ctrl.bps.BpsConfig` | ||
| BPS configuration. | ||
| out_prefix : `str` or None | ||
| out_prefix : `str` |
There was a problem hiding this comment.
Again, the qualifier optional is missing.
doc/lsst.ctrl.bps/quickstart.rst
Outdated
| **whenCreate** | ||
| When during the submission process that the execution butler is created. | ||
| whenCrete valid values: "NEVER", "ACQUIRE", "TRANSFORM", "PREPARE", | ||
| "SUBMIT" |
There was a problem hiding this comment.
With the current formatting, Sphinx will treat "SUMBIT" as a new entry in the description and I suspect (judging from the text itself) that's not the case.
There was a problem hiding this comment.
This file was updated since this comment was made.
| Filtered executable names or objects from generic workflow. | ||
| """ | ||
| execs = [] | ||
| for name, executable in self._execs.items(): |
There was a problem hiding this comment.
Unless I'm missing something, it doesn't look like the GenericWorkflow has the _execs attribute. Did you mean self._executables?
| @@ -441,6 +450,8 @@ class HTCJob: | |||
| Initial commands for job inside DAG. | |||
| initattrs : `dict` | |||
| Initial dictionary of job attributes. | |||
| subfile : `str` | |||
| Filename for the job's submit script. | |||
There was a problem hiding this comment.
It doesn't look like this parameter is used in the class constructor.
Also added text=auto in .gitattributes and fixed the line endings in a couple files. Fixed some docstrings (either missing or just missing parameters). Removed some parameters from internal functions that were no longer using them.
MichelleGower
force-pushed
the
tickets/DM-28653
branch
from
24eecbf
to
c278fc2
Compare
Separate GenericWorkflowJob's cmdline into executable and arguments where executable now a GenericWorkflowExec. Also per meeting discussion, make execution butler not the default. Fix bpsUseShared bugs and bug when creating merge job. Updated quickstart guide. Misc. updates based upon review comments.
MichelleGower
force-pushed
the
tickets/DM-28653
branch
from
b3cb1eb
to
92715a8
Compare
timj
changed the title
Creating a draft pull request for the initial changes.