Skip to content

fix: use receiver timestamps for completion events - #537

Open
wsj040303 wants to merge 3 commits into
mlcommons:mainfrom
wsj040303:fix/stream-completion-timing
Open

wsj040303 wants to merge 3 commits into
mlcommons:mainfrom
wsj040303:fix/stream-completion-timing

Conversation

@wsj040303

@wsj040303 wsj040303 commented Oct 14, 2026 •

Copy link
Copy Markdown

What does this PR do?

Use the receiver's now_ns for COMPLETE events, matching RECV_FIRST, RECV_NON_FIRST and ERROR events. A single socket drain can decode a QueryResult before the session handles its preceding chunks. Using its decode-time completed_at then stamps completion before the first chunk and produces negative TPOT.

The QueryResult timestamp and wire format are unchanged. The session continues reading the monotonic clock once per response. Completion now includes the same receiver-handling boundary that chunk events already use.

Type of change

  • Bug fix

Related issues

Closes #533.

Testing

  • Tests added/updated
  • Manual testing completed

Four batched-response regressions (one/multiple chunks, successful/failed terminal results) fail on the original source and pass after the fix. The existing metadata/turn test now checks the shared receiver timestamp.

  • pytest tests/unit/load_generator/test_async_session.py -q --no-cov: 65 passed.
  • LC_ALL=C.UTF-8 pytest -m unit -q: 2259 passed, 6 skipped. The initial run's one failure expected an English shell error under the host's Chinese locale; the C-locale rerun passes.
  • Offline, Poisson and concurrency HTTP benchmarks, streaming on/off: 6 passed. These use the existing echo server and the repository's local char_chat tokenizer fixture under a temporary echo-server alias. The alias was removed after testing. An inherited SOCKS proxy initially prevented the tokenizer subprocess from starting; the successful run removed proxy variables for local testing.
  • A real ZMQ worker connector sent a queued chunk/chunk/result batch through the production receiver; RECV_FIRST <= RECV_NON_FIRST <= COMPLETE.
  • pre-commit run --all-files passed, including mypy (335 files), Ruff, formatting, license and generated-template checks. git diff --check passed.

External serving and optional hardware/slow suites were not run. No model weights or GPU are required for this client event-timing fix.

Checklist

  • Code follows project style
  • Pre-commit hooks pass
  • Documentation updated (if needed): the existing receiver-timestamp contract now matches implementation.

Dependency CI follow-up (2026-10-23)

Merged upstream main at 93ec321 (#542) to resolve the dependency audit failure. The original timing fix and its regression tests are unchanged. Locally: dependency audit passes with the existing #520 exception, 2,259 unit tests pass (6 skipped), and all pre-commit hooks pass. New head: 238cfe5. The CLA and reviewer-request permissions remain separate checks.

Signed-off-by: wsj <279978307+wsj040303@users.noreply.github.com>
@wsj040303
wsj040303 requested a review from a team October 14, 2026 06:56
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

MLCommons CLA bot:
Thank you very much for your submission; we really appreciate it. Before we can accept your contribution,
we ask that you sign the MLCommons CLA (Apache 2). Please submit your GitHub ID to our onboarding form to initiate
authorization. If you are from a MLCommons member organization, we will request that you be added to the CLA.
If you are not from a member organization, we will email you a CLA to sign. For any questions, please contact
support@mlcommons.org.
1 out of 2 committers have signed the MLCommons CLA.
✅ @arekay-nv
❌ @wsj040303
You can retrigger this bot by commenting recheck in this Pull Request

@github-actions github-actions Bot added the size/normal PR Review Policy: <=500 non-test lines & <=20 files label Oct 14, 2026
@arekay-nv

Copy link
Copy Markdown
Collaborator

@wsj040303 can you please sign the CLA.

Thank you very much for your submission; we really appreciate it. Before we can accept your contribution,
we ask that you sign the MLCommons CLA (Apache 2). Please submit your GitHub ID to our onboarding form to initiate
authorization. If you are from a MLCommons member organization, we will request that you be added to the CLA.
If you are not from a member organization, we will email you a CLA to sign. For any questions, please contact
support@mlcommons.org.
0 out of 1 committers have signed the MLCommons CLA.
❌ @wsj040303

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@d3c57f2). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #537   +/-   ##
=======================================
  Coverage        ?   81.37%           
=======================================
  Files           ?      157           
  Lines           ?    22472           
  Branches        ?        0           
=======================================
  Hits            ?    18286           
  Misses          ?     4186           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: wsj <279978307+wsj040303@users.noreply.github.com>
@wsj040303

Copy link
Copy Markdown
Author

The audit failure is caused by the older dependency pins, which upstream has already fixed in #542. I merged main at 93ec321 into this branch without changing the timing fix. Locally, the original dependency audit failed; after the merge, the audit passes with the existing #520 exception, 2,259 unit tests pass (6 skipped), and all pre-commit hooks pass. The branch now points to 238cfe5.

The auto-review log separately reports HTTP 403 when requesting reviewers, and the CLA check remains outstanding; this dependency update does not resolve those checks.

@arekay-nv

Copy link
Copy Markdown
Collaborator

@wsj040303 please complete the CLA so we can review the PR.

MLCommons CLA bot: Thank you very much for your submission; we really appreciate it. Before we can accept your contribution, we ask that you sign the MLCommons CLA (Apache 2). Please submit your GitHub ID to our onboarding form to initiate authorization. If you are from a MLCommons member organization, we will request that you be added to the CLA. If you are not from a member organization, we will email you a CLA to sign. For any questions, please contact support@mlcommons.org.1 out of 2 committers have signed the MLCommons CLA.✅ @arekay-nv❌ @wsj040303You can retrigger this bot by commenting recheck in this Pull Request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/normal PR Review Policy: <=500 non-test lines & <=20 files

Projects

None yet

3 participants