DM-53494: Add code needed to run submit processes as jobs. - #83
DM-53494: Add code needed to run submit processes as jobs.#83MichelleGower wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #83 +/- ##
==========================================
+ Coverage 85.26% 88.06% +2.80%
==========================================
Files 19 21 +2
Lines 4417 4994 +577
Branches 463 498 +35
==========================================
+ Hits 3766 4398 +632
+ Misses 575 506 -69
- Partials 76 90 +14 ☔ View full report in Codecov by Harness. |
575bff3 to
998b35b
Compare
998b35b to
7bad3fc
Compare
9bc26a2 to
df403d1
Compare
331b9e1 to
e40b7c5
Compare
Note: Limited to same cluster with shared filesystems. Includes handling of lazy subdags.
e40b7c5 to
a443586
Compare
a443586 to
37d9867
Compare
| --------------------------- | ||
|
|
||
| When BPS uses batch jobs for the submission process, there isn't much | ||
| different in how HTC handles them vs the payload jobs. There will be |
There was a problem hiding this comment.
How about changing
When BPS uses batch jobs for the submission process, there isn't much different in how HTC handles them vs the payload jobs.
to
When BPS uses batch jobs for submission, HTC handles them much the same way it handles payload jobs.
|
|
||
| Noticable differences in the HTCondor-level details include a second | ||
| ``*.dag`` file in the submit directory and a second ``condor_dagman`` | ||
| job in the queue. This is subDAG handles the payload workflow and |
There was a problem hiding this comment.
"This is subDAG handles" -> "This sub-DAG handles"
|
|
||
|
|
||
| def read_dag_info(wms_path: str | os.PathLike) -> dict[str, dict[str, Any]]: | ||
| def read_dag_info(wms_path: str | os.PathLike) -> tuple[str, dict[Path, dict[str, Any]]]: |
There was a problem hiding this comment.
Shouldn't the type hint for the return value be tuple[Path, dict[str, dict[str, Any]]]?
| rescue_dags = list(path.glob("*.rescue[0-9][0-9][0-9]")) | ||
| # If using control DAG, don't want to include nested DAGs. | ||
| rescue_dags = list(path.glob("*_ctrl.dag.rescue[0-9][0-9][0-9]")) | ||
| if len(rescue_dags) == 0: |
There was a problem hiding this comment.
Can we rewrite this condition as:
if not rescue_dags:
...| Returns | ||
| ------- | ||
| filename : `pathlib.Path` | ||
| Name of the file where the information was written. |
There was a problem hiding this comment.
I don't mind changing the order of the parameters. However, I don't understand why you made the filename parameter optional and needed to include some ad hoc mechanism for creating the name for the output file if it is not provided. I think making the caller always responsible for generating the output filename will result in the code that is easier to follow. After all, write_dag_info(info, filename) convey the information what is written where much better than just write_dag_info(info).
Also, this function appears three times in the plugin's code. Unless I missed something, only a single call uses the fact that filename is an optional parameter now (line 184 in htcondor_service.py). So I don't see any reason why generating the filename can't happen there, in the caller, as in other two cases.
| placeholder_dag = HTCDag(name=placeholder_dag_name) | ||
| _LOG.debug("dag name = %s", placeholder_dag.graph["name"]) | ||
| placeholder_dag.add_attribs(generic_workflow.run_attrs) | ||
| placeholder_dag.add_job(job1) |
There was a problem hiding this comment.
Can we rename job1 to just job?
2 participants
Note: Limited to same cluster with shared filesystems. Includes handling of lazy subdags.
Checklist
doc/changes