Conversation
FloChehab
force-pushed
the
fix/summary-v2
branch
2 times, most recently
from
2394338 to
2cf2e4e
Compare
cameledev
left a comment
cameledev
left a comment
There was a problem hiding this comment.
Good catch. As far as I have seen, it seems like numbers, never have start / stop.
| logger.info( | ||
| "Transcribe task %s failed, %s retries left.", | ||
| task_id, | ||
| n_retries_left, | ||
| ) |
There was a problem hiding this comment.
This message can be a bit confusing. If I understand properly, it will say "0 retries left." and then retry.
There was a problem hiding this comment.
Nope, it will not retry.
| n_retries_left = sender.max_retries - sender.request.retries - 1 | ||
| if n_retries_left <= 0: |
| else: | ||
| logger.info( | ||
| "Summary task %s failed, %s retries left.", | ||
| task_id, | ||
| n_retries_left, | ||
| ) |
| if task.request.retries >= task.max_retries: | ||
| n_retries_left = sender.max_retries - sender.request.retries - 1 | ||
| if n_retries_left <= 0: |
There was a problem hiding this comment.
This changes the current behavior. Is this intentional ?
ex:
- task.request.retries = 1
- task.max_retries = 2
There was a problem hiding this comment.
If retries = 1, it means that the task has been tried twice. And in your case it shouldn't be retry anymore (so we are good from my pov). There was an off by one error before.
FloChehab
force-pushed
the
fix/summary-v2
branch
from
2cf2e4e to
6ebb623
Compare
lebaudantoine
left a comment
lebaudantoine
left a comment
There was a problem hiding this comment.
Overall, reviewing this PR gives me the impression that we should further battle-test the recent changes in the staging environment before considering a release. There is a significant risk of introducing regressions, as several refactored components were not tested.
I suggest we schedule a few sessions on the staging environment with transcription enabled to validate behavior and catch potential issues early.
FloChehab
force-pushed
the
fix/summary-v2
branch
from
6ebb623 to
f118b76
Compare
Computation was off by 1. Also improve the logging.
Sometimes whisperX response is partial, we don't want to crash in such case.
FloChehab
force-pushed
the
fix/summary-v2
branch
from
f118b76 to
451be40
Compare
|
3 participants
Quick fixes on the new summary v2 tasks (see commits).