DM-41543: Adds Reporting Exit Code Information to bps report #157
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
- Coverage 78.70% 78.60% -0.10%
==========================================
Files 40 40
Lines 3090 3151 +61
Branches 519 530 +11
==========================================
+ Hits 2432 2477 +45
- Misses 568 582 +14
- Partials 90 92 +2 ☔ View full report in Codecov by Sentry. |
villarrealas
force-pushed
the
tickets/DM-41543
branch
3 times, most recently
from
57e7d28
to
717c01e
Compare
python/lsst/ctrl/bps/wms_service.py
Outdated
| """Job counts per label and per state. | ||
| """Job counts per label and per state.""" | ||
|
|
||
| exit_code_summary: list[set] = None |
There was a problem hiding this comment.
I think this type description may be incorrect. Unless I got it wrong, it will be a list of lists (see lines 189, 192, 213, and 241 in ctrl_bps_panda/python/ctrl/bps/panda/panda_service.py). Also, it would be nice to have type specification for the elements for these list as well (list[list[int]]?).
However, to be perfectly honest, I'd prefer exit_code_summary to be a dictionary mapping job labels to list/sets of their exit codes (similarly to the job_summary mapping job labels to job states and their counts). With a dictionary a client code doesn't need to worry if the ordering of the exit_code_summary corresponds to the ordering of job labels somewhere else.
python/lsst/ctrl/bps/bps_reports.py
Outdated
| def __str__(self): | ||
| alignments = ["<"] + [">"] * (len(self._table.colnames) - 1) | ||
| lines = list(self._table.pformat_all(align=alignments)) | ||
| # lines.insert(3, lines[1]) |
There was a problem hiding this comment.
Please remove the line you commented out if you don't need it.
python/lsst/ctrl/bps/bps_reports.py
Outdated
| labels = [] | ||
| if run_report.run_summary: | ||
| for part in run_report.run_summary.split(";"): | ||
| label, count = part.split(":") |
There was a problem hiding this comment.
Variable count doesn'st seem to be used for anything. Please replace with _. Though before making this change please see the comment in ctrl/bps/wms_service.py regarding the type of exit_code_summary first. If you make the changes I'm suggesting, this cooment will likely be rendered moot.
python/lsst/ctrl/bps/wms_service.py
Outdated
|
|
||
| exit_code_summary: list[set] = None | ||
| """Summary of exit codes provided by the WMS if | ||
| available. |
There was a problem hiding this comment.
Can we use something a little bit more descriptive here, e.g. "Non-zero exit codes per job label if available."?
Also, if you don't use a dictionary for exit_code_summary as I suggested earlier please include the information about expected ordering of the list.
doc/changes/DM-41543.feature.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Introduced the `--return-exit-codes` flag to bps report, which provides a summary of exit code counts and exit codes for non-payload errors. This currently only works for PanDA. | |||
There was a problem hiding this comment.
I'm not really sure if we can use Markdown markup in these RST documents so just to be safe I would use double backquotes around --return-exit-codes.
| ] | ||
| run_exits_report = ExitCodesReport(fields) | ||
| run_exits_report.add(run, use_global_id=is_global) | ||
| print(run_exits_report) |
There was a problem hiding this comment.
Can we add print("\n") before this line so the job report and the exit code report are separated by a blank line if the latter is displayed?
| exit_code_summary = run_report.exit_code_summary | ||
| for label, exit_codes in zip(labels, exit_code_summary): | ||
| if exit_codes != [None]: | ||
| pipe_error_count = sum([code for code in exit_codes if code == 1]) |
There was a problem hiding this comment.
Maybe it would be worthwhile adding a comment that we're taking advantage of the fact that payloads always return 1 on failure here (right?).
villarrealas
force-pushed
the
tickets/DM-41543
branch
from
a9cad42
to
85cbcea
Compare
Preliminary draft version to run some things past Michelle since it is my first significant contribution to BPS code base.
Checklist
doc/changes