Skip to content

Fix stale start timestamp in timestamp_sentence_en when timestamp list is shorter than punctuation list - #3741

Merged
LauraGPT merged 1 commit into
modelscope:mainfrom
Arthur031221:fix/timestamp-sentence-en-stale-start
Sep 30, 2026
Merged

LauraGPT merged 1 commit into
modelscope:mainfrom
Arthur031221:fix/timestamp-sentence-en-stale-start

Conversation

@Arthur031221

Copy link
Copy Markdown
Contributor

Summary

timestamp_sentence_en in funasr/utils/timestamp_tools.py uses a one-shot is_sentence_start flag to decide when to record a new sentence's start timestamp. The flag fires exactly once right after a sentence is flushed, regardless of whether the current timestamp is None. When punc_id_list and timestamp_postprocessed differ in length (a case the code already detects and logs a warning for), itertools.zip_longest pads the missing trailing entries with timestamp=None. If the first token of a new sentence lands on one of these padded None entries, is_sentence_start is cleared without ever storing a real start value, so sentence_start keeps whatever value was left from the previous, unrelated sentence. The sibling function timestamp_sentence (used for Chinese/default text) avoids this: it resets sentence_start = None after each flush and only sets it once a non-None timestamp actually appears, retrying on every iteration until one does.

This PR replaces the one-shot flag in timestamp_sentence_en with the same reset-and-retry pattern already used by timestamp_sentence. No other behavior changes.

Repro before the fix, with punc_id_list = [1, 3, 3], timestamp_postprocessed = [[0, 100], [100, 200]] (one entry short), text_postprocessed = "hello world bar":

{'text': 'bar.', 'start': 0, 'end': 200, 'timestamp': [None]}

start is copied from the first sentence ("hello world.", also start=0, end=200), so the two unrelated sentences get identical timestamps instead of the second one being flagged unknown. timestamp_sentence on the same shape of input correctly returns start=None.

After the fix, timestamp_sentence_en returns start=None for that sentence, matching the Chinese sibling.

Type of change

  • Bug fix
  • Documentation
  • Example or demo
  • Runtime or deployment
  • Benchmark or evaluation
  • Model/training change

Validation

  • python -m compileall funasr examples tests -- exit 0
  • python -m pytest tests/test_timestamp_tools.py -q -- 3 passed (2 pre-existing + 1 new regression test)
  • Reverted funasr/utils/timestamp_tools.py to the pre-fix commit and reran the same test file: the new test fails with assert 0 is None, confirming it catches the bug. The two pre-existing tests still pass on the old code. Restored the fix afterward.
  • Full local suite: 579 passed, 11 skipped, 4 failed. The 4 failures are pre-existing and unrelated to this change (a headless-browser docs rendering check, a GitHub Pages workflow-path assertion, and a missing optional uvicorn dependency in the local environment). None touch funasr/utils/timestamp_tools.py or any file this PR changes.
  • Docs or links checked -- not applicable
  • Runtime/deployment command tested -- not applicable

Added tests/test_timestamp_tools.py::test_timestamp_sentence_en_missing_trailing_timestamp_matches_zh_fallback, which feeds the same repro inputs to both timestamp_sentence_en and timestamp_sentence and asserts the second sentence's start is None in both, so they agree.

User impact

Anyone calling generate() with sentence_timestamp=True and en_post_proc=True (English/latin sentence-level timestamps, e.g. subtitle or caption generation in funasr/auto/auto_model.py) hits this whenever the punctuation model's token count and the ASR timestamp count diverge for a batch. That mismatch is already logged as a warning elsewhere in the code, so it's a known, occurring condition rather than a contrived edge case. Before this fix the output silently carried a wrong, duplicated start time for the affected sentence instead of None, which is worse than a crash since it looks like valid data downstream.

Notes for reviewers

Change is scoped to timestamp_sentence_en; timestamp_sentence is untouched. Behavior for the common case (matching-length lists) is unchanged, verified by the two pre-existing tests still passing.

When punc_id_list is longer than timestamp_postprocessed (a mismatch
the function already logs a warning for), the entries past the end of
timestamp_postprocessed get timestamp=None from zip_longest. For the
sentence that starts on one of those entries, the is_sentence_start
flag was consumed unconditionally on the first iteration regardless of
whether timestamp was None, so sentence_start kept whatever value was
left over from the previous, unrelated sentence instead of being
reported as unknown.

The sibling function timestamp_sentence already handles this correctly
by resetting sentence_start to None after each flush and only setting
it once a real timestamp shows up. Mirror that logic here instead of
the one-shot is_sentence_start flag.

Before: a trailing sentence whose first word has no timestamp gets
start reused from the previous sentence (wrong, looks valid).
After: start is None for that sentence, matching timestamp_sentence.

@LauraGPT LauraGPT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independently verified the regression at 12817ec using the real FunASR module in the native CPU test environment.

  • Ran the new three-test file against its parent 7f85ddd: 1 failed, 2 passed. The new case fails specifically with assert 0 is None.
  • Ran python -m pytest -q tests/test_timestamp_tools.py on this PR head: 3 passed.
  • The reset/retry logic now agrees with timestamp_sentence for the missing trailing timestamp, while the existing normal-output and whitespace regressions still pass. I found no blocking issue in this scoped change.

This verification is limited to these timestamp unit tests. I have not independently reproduced or classified the four full-suite failures mentioned in the PR description. No model download, inference, or hardware validation was performed.

@Arthur031221

Copy link
Copy Markdown
Contributor Author

Thanks for the independent verification. Re-ran tests/test_timestamp_tools.py just now on this PR's head commit: 3 passed, matching your result.

On the failures: the PR description's "Full local suite" line is this same run, not all 113 test files. It is 68 files, grep-selected for not needing network access or model download, then trimmed by 5 more that fail to import in this environment (three need markdown, two need datasets), run with HF_HUB_OFFLINE=1. That gives four failures across three tests. A plain pytest tests in this environment stops earlier still, on 12 collection errors from the same two missing dependencies.

The three tests are tests/test_generated_api_docs.py::GeneratedApiDocsTests::test_browser_navigation_and_responsive_overflow (two subfailures, a headless-browser viewport check), tests/test_gh_pages_moss_publication.py::test_github_pages_moss_source_is_versioned_and_discoverable (asserts a literal path inside .github/workflows/update-api-docs.yml), and the n8n Whisper-alias compatibility test for the example server (test_example_server_maps_n8n_whisper_alias_to_started_model, missing uvicorn, an optional dependency for that example, in the test environment). None of the three failing tests import or exercise timestamp_tools.py, and all four reproduce the same way on the pre-fix commit, so they are unrelated to this change.

@LauraGPT LauraGPT left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rechecked exact head 12817ecdbb1037394e28e9474aab55897cf84637 against current main 7b098acefc22999f1be483b7b6fca62db8c8ad39.

The focused timestamp suite passes 3/3 on both the submitted head and a clean synthetic merge; compileall and git diff --check also pass. The previously recorded parent control fails only the new regression (1 failed, 2 passed), confirming that the test discriminates the stale-start bug. The reset-and-retry behavior is scoped and matches the sibling timestamp_sentence fallback.

Approving.

@LauraGPT
LauraGPT merged commit be7557f into modelscope:main Sep 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants