DM-55232: fix zstd failure in aggregate-graph due to huge logs#565
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
==========================================
- Coverage 88.73% 88.67% -0.07%
==========================================
Files 160 160
Lines 22120 22151 +31
Branches 2625 2627 +2
==========================================
+ Hits 19629 19642 +13
- Misses 1849 1866 +17
- Partials 642 643 +1 ☔ View full report in Codecov by Harness. |
TallJimbo
force-pushed
the
tickets/DM-55232
branch
from
07e4ee7 to
b017815
Compare
MichelleGower
left a comment
MichelleGower
left a comment
There was a problem hiding this comment.
A couple comments. Merge approved.
| break | ||
| if training_sample_size >= self.comms.config.zstd_dict_input_max_bytes: | ||
| self.comms.log.warning( | ||
| "Truncating compression dict training sample after %d predicted quanta.", |
There was a problem hiding this comment.
I don't know if it would be quicker to understand if instead of saying "Truncating" saying something like "Reached compression dict training sample limit after..."
There was a problem hiding this comment.
Also, do you want a similar warning with the above break for counts?
There was a problem hiding this comment.
I've also reworked the logic break before exceeding the limit, rather than just after.
| pred_quanta_sum = sum(len(block) for block in training_inputs[:n_pred_quanta]) | ||
| prov_quanta_sum = sum(len(block) for block in training_inputs[n_pred_quanta::3]) | ||
| metadata_sum = sum(len(block) for block in training_inputs[n_pred_quanta + 1 :: 3]) | ||
| logs_sum = sum(len(block) for block in training_inputs[n_pred_quanta + 2 :: 3]) |
There was a problem hiding this comment.
I possibly would set the step (3) in a variable to make sure future changes get them all. But the lines are all here together so...
If it is easy to write a unit test to catch if we add/remove something to/from the training_inputs but forget to change the step, that could avoid trying to track down a bug in the exception handling code.
There was a problem hiding this comment.
This case should be rare enough and the test looks tricky enough that I'm going to skip it.
TallJimbo
force-pushed
the
tickets/DM-55232
branch
from
b017815 to
e8f27a2
Compare
TallJimbo
force-pushed
the
tickets/DM-55232
branch
from
e8f27a2 to
ad03ba2
Compare
2 participants
Checklist
package-docs builddoc/changes