DM-27570: Fix bps report ordering and history-loss issues. #22
Conversation
| @@ -239,7 +236,7 @@ def write(self, out_prefix): | |||
| self.dag.write(out_prefix, "jobs/{self.label}") | |||
|
|
|||
|
|
|||
| def translate_job_cmds(generic_workflow_job): | |||
| def _translate_job_cmds(generic_workflow_job): | |||
There was a problem hiding this comment.
I'm not quite convinced that you really need the leading underscore to emphasize the fact that this function is "private". The list of public objects in the module (__all__) does not include it (and others) anyway. I mean nothing is technically wrong with having the underscore, so you may leave it as is. It just feels like an overkill.
There was a problem hiding this comment.
I'm fine with the underscore. Python lets people import things that aren't in the __all__ and the leading underscore really does tell people they get everything they deserve if they import it anyhow.
| return run_reports | ||
|
|
||
|
|
||
| def summary_report(user, hist, pass_thru): |
There was a problem hiding this comment.
Why doesn't it start with the leading underscore like the others? It's not included in __all__ and it doesn't look like it's used outside of this module anyway.
| return wms_state | ||
|
|
||
|
|
||
| def update_jobs(jobs1, jobs2): |
There was a problem hiding this comment.
The same question as in the case of summary_report.
|
|
||
| _LOG = logging.getLogger() | ||
| COUNT_ERROR = -99999 |
There was a problem hiding this comment.
Doesn't look like this "constant" is a part of module's public interface. Shouldn't it start with the leading underscore then?
There was a problem hiding this comment.
Found a use case where this caused problems, so will be removing it completely.
Also tweaked some of the column widths (and truncating too long values) as well as switched init to pipetaskInit in report to match submit yaml.
For the three files in which this ticket made changes, fixed the quote inconsistencies. Also, fixed any inconsistencies in the non-public functions regarding leading underscore and _all_. Found a couple more reporting situations where output wasn't correct and fixed those.
MichelleGower
force-pushed
the
tickets/DM-27570
branch
from
52c1a68
to
c991a1d
Compare
Fixed the order of the reporting of pipelineTasks to be consistent between calls. Also fixed the reporting in the summary of running jobs (would always be 0). And because jobs were falling off of the NCSA history logs so fast, bps report was treating those forgotten jobs as unready. Fixed it to use more information from the submit directory (but it is slower for large DAGs).
Also tweaked some of the column widths (and truncating too long values) as well as switched init to pipetaskInit in report to match submit yaml.