Fix crash on VTT timestamps at or past 24:00:00 - #9
Open
abhishekKokadwar wants to merge 1 commit into
Open
abhishekKokadwar wants to merge 1 commit into
abhishekKokadwar wants to merge 1 commit into
Conversation
datetime(2000, 1, 1, hour, ...) raises ValueError when hour >= 24, which happens for long-form YouTube auto-captions (multi-hour streams/lectures) that use timestamps like 24:xx:xx or higher. Replaced the datetime-based +10ms calculation with plain integer arithmetic on total milliseconds, which has no upper bound on hours.
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the English VTT→CoNLL-U conversion script when processing long-form WebVTT cues whose timestamps reach or exceed 24:00:00, ensuring the pipeline can continue over large corpora without aborting on multi-hour caption files.
Changes:
- Replaced
datetime/timedeltatimestamp math with an unbounded millisecond arithmetic helper (add_10ms()). - Updated end-time computation to use
add_10ms()for cue end timestamps. - Added a small standalone self-check script to validate rollover and the >=24h regression case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| english/vtt_auto_to_conll-u.py | Removes datetime-based time arithmetic and uses add_10ms() to avoid hour=24+ crashes. |
| english/test_vtt_auto_to_conll_u.py | Adds a lightweight, dependency-free self-check for the new timestamp rollover logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+5
to
+7
| # Plain seconds arithmetic instead of datetime() -- datetime's hour field | ||
| # is capped at 23, but long VTT files (multi-hour streams/lectures) can | ||
| # have timestamps at or past 24:00:00. |
Comment on lines
+9
to
+13
| def add_10ms(timestamp): | ||
| h, m, s_ms = timestamp.split(":") | ||
| s, ms = s_ms.split(".") | ||
| total_ms = ((int(h) * 3600 + int(m) * 60 + int(s)) * 1000 + int(ms)) + 10 | ||
| h, rem_ms = divmod(total_ms, 3600000) |
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
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.
Bug
english/vtt_auto_to_conll-u.pycomputes each token's end timestamp with:datetime()raisesValueError: hour must be in 0..23whenever the VTT timestamp's hour is 24 or higher. This is a real case for long-form YouTube auto-captions (multi-hour streams, lectures, etc.), where cue timestamps like24:xx:xx.xxxor higher are valid. Sinceconvert_vtt_to_conll-u.shloops over every*en.vttfile in a corpus directory, a single long file crashes that iteration of the pipeline with an unhandled exception.Fix
Replaced the
datetime-based +10ms calculation with plain integer arithmetic in milliseconds (add_10ms()), which has no upper bound on the hour component. Behavior for all timestamps under 24 hours is unchanged.Testing
Added
english/test_vtt_auto_to_conll_u.py, a small standalone self-check (no external deps, since the main script requiressomajowhich isn't installed outside the pipeline's HPC environment). It verifies normal rollover cases plus the regression case (23:59:59.995->24:00:00.005) that used to crash.I'm a prospective GSoC 2027 contributor exploring this pipeline — happy to help with anything else that would be useful here.