DM-38377: Fix label order bugs found by rescue workflows. #139
Conversation
MichelleGower
force-pushed
the
tickets/DM-38377
branch
from
989e5ce
to
0ebaf80
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
+ Coverage 78.65% 80.00% +1.34%
==========================================
Files 39 40 +1
Lines 2863 3015 +152
Branches 489 511 +22
==========================================
+ Hits 2252 2412 +160
+ Misses 535 514 -21
- Partials 76 89 +13
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
|
||
| class GenericWorkflowLabels: | ||
| """A generic representation of a workflow used to submit to specific | ||
| workflow management systems. |
There was a problem hiding this comment.
The docstring needs to be updated.
python/lsst/ctrl/bps/transform.py
Outdated
| @@ -181,7 +182,7 @@ def create_init_workflow(config, qgraph, qgraph_gwfile): | |||
| init_workflow.add_file(qgraph_gwfile) | |||
|
|
|||
| # create job for executing --init-only | |||
| gwjob = GenericWorkflowJob("pipetaskInit") | |||
| gwjob = GenericWorkflowJob("pipetaskInit", "pipetaskInit") | |||
There was a problem hiding this comment.
I'd rewrite it as
gwjob = GenericWorkflowJob("pipetaskInit", label="pipetaskInit")
to improve readability.
| if not self._label_to_jobs[job.label]: | ||
| del self._label_to_jobs[job.label] | ||
|
|
||
| # remove from graph |
There was a problem hiding this comment.
This comment seems a little bit redundant. I'd remove it.
tests/cqg_test_utils.py
Outdated
| cqg = ClusteredQuantumGraph("cqg1", qgraph, f"{outdir}/test_file.qgraph") | ||
|
|
||
| # since random hash ids, create mapping for tests | ||
| test_lookup = {} # since random hash ids, provide mapping for tests |
There was a problem hiding this comment.
Repeated comment, please remove one of them.
| Directed graph of tasks. | ||
|
|
||
| Returns | ||
| ------- | ||
| task_labels : `set` [`str`] | ||
| Set of task labels from the cluster definitions. | ||
| cluster_labels: `list` |
There was a problem hiding this comment.
It would be nice to specify the type of the elements as well.
| # T2(1,2) T2(3,4) | ||
| # | | | ||
| # T3(1,2) T3(3,4) | ||
| def make_test_clustered_quantum_graph(outdir): |
There was a problem hiding this comment.
As this function is (sort of) a part of the public API, adding a docstring would be nice.
| quantum_to_cluster : `dict` [ `str`, `str` ] | ||
| Mapping of quantum node id to which cluster it was added. | ||
| (modified in method) | ||
| """ |
There was a problem hiding this comment.
The docstring doesn't mentioned that the function raises KeyError. Please add the relevant section.
| for tdef in task_graph: | ||
| label_graph.add_node(tdef.label) | ||
| for parent in task_graph.predecessors(tdef): | ||
| label_graph.add_edge(parent.label, tdef.label) |
There was a problem hiding this comment.
Just FYI, you could potentially replace lines 125-127 with
label_graph.add_edges_from([(parent.label, tdef.label) for parent in task_graph.predecessors(tdef)])As a side note, you could even "collapse" lines 124-127 to
label_graph.add_edges_from([(parent.label, tdef.label) for tdef in task_graph for parent in task_graph.predecessors(tdef)])but I feel like it actually make code less readable in this case. So I'm leaving the final decision up to you (which may include making no changes whatsoever).
There was a problem hiding this comment.
I'm going to leave the code the way it is for this ticket.
MichelleGower
force-pushed
the
tickets/DM-38377
branch
from
0ebaf80
to
a02a86b
Compare
MichelleGower
force-pushed
the
tickets/DM-38377
branch
from
a02a86b
to
8476afe
Compare
Checklist
doc/changes