fix: subtitle timestamp ms overflow; checkpoint KeyError on manifest-only stages#325
Open
0xDevNinja wants to merge 2 commits into
Open
Conversation
_ts_srt/_ts_vtt computed the seconds and millisecond fields independently: `ms = int(round((seconds % 1) * 1000))`. When the fractional part is >= 0.9995 that rounds to 1000, emitting a malformed 4-digit `…,1000` value with no carry into the seconds field (and, at 59.9999/3599.9999, no carry into minutes/hours). For example 0.9999s became `00:00:00,1000` instead of `00:00:01,000`. ASR word and segment end-times routinely land on such fractional boundaries, and the resulting cue is rejected or mistimed by strict SRT/VTT parsers (ffmpeg subtitles filter, VLC, browser WebVTT). Decompose from a single rounded total-milliseconds value so the carry propagates across all fields. Both formatters now share one `_hmsms` helper.
…l artifact _validate_artifacts_for_stage looked up CANONICAL_STAGE_ARTIFACTS[stage] unconditionally, but the valid stage list comes from the pipeline manifest via get_pipeline_stages(), which declares stages beyond the 9 canonical ones — e.g. character-animation adds `character_design`/`rig_plan`. Such a stage passes the `stage in valid_stages` guard, then raised an unhandled KeyError on the canonical lookup, so those stages could never be checkpointed (the crash hits write/read_checkpoint and friends, even for in_progress checkpoints). Look the canonical artifact up defensively with `.get()` and skip the required-artifact check when there is none. Canonical stages still require their artifact when completed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent defects that corrupt output / crash on ordinary inputs. Separate atomic commits.
1.
subtitle_gen— millisecond overflow (commit 1)_ts_srt/_ts_vttcomputed seconds and ms independently (ms = round((seconds % 1) * 1000)), which rounds to1000when the fraction is >= 0.9995 — emitting a malformed 4-digit…,1000with no carry into the seconds/minutes/hours field.0.9999s -> 00:00:00,1000(should be00:00:01,000);59.9999 -> 00:00:59,1000(should be00:01:00,000). ASR word/segment end-times hit these boundaries routinely, and the cue is rejected/mistimed by strict SRT/VTT parsers (ffmpegsubtitles, VLC, WebVTT). Fixed by decomposing from a single rounded total-ms value via a shared_hmsmshelper so the carry propagates.2.
checkpoint— KeyError on manifest-only stages (commit 2)_validate_artifacts_for_stagedid an unconditionalCANONICAL_STAGE_ARTIFACTS[stage], but valid stages come from the pipeline manifest (get_pipeline_stages), which declares stages beyond the 9 canonical ones (e.g.character-animationscharacter_design/rig_plan). Those pass thestage in valid_stagesguard, then raise an unhandledKeyError, so the stage can never be checkpointed. Fixed with a defensive.get()that skips the required-artifact check when the stage has no canonical artifact; canonical stages still require theirs when completed.Tests
tests/tools/test_subtitle_timestamps.py— ms carry across second/minute/hour boundaries, normal values unchanged, all outputs well-formedHH:MM:SS,mmm.tests/lib/test_checkpoint_noncanonical_stage.py— manifest-only stage validates without KeyError; canonical stage still requires its artifact; premise guarded (manifest really declares the stage).Confirmed the behavioral tests fail on
mainbefore the fix (checkpoint raisesKeyError: character_design).Closes #324