Skip to content

test(embeddings): restore skip_for_runtime("sglang") on correctness test - #1330

Merged
slin1237 merged 1 commit into
mainfrom
revert/1326-sglang-embedding-skip
Apr 22, 2026
Merged

slin1237 merged 1 commit into
mainfrom
revert/1326-sglang-embedding-skip

Conversation

@slin1237

@slin1237 slin1237 commented Apr 22, 2026 •

Copy link
Copy Markdown
Member

Summary

Reverts #1326 to restore the @pytest.mark.skip_for_runtime("sglang", reason="sglang embedding output diverges from HF reference") decorator on test_correctness.py and put the embeddings matrix back on the 1-gpu runner pool.

Why

#1326 removed the skip on the hypothesis that switching to H100 hardware (also introduced in #1326) would close the sglang vs HuggingFace reference gap for intfloat/e5-mistral-7b-instruct. Empirically it does not:

  • PR fix(ci): install ffmpeg OS libs for embeddings extras #1329 CI (commit e39ede16, sglang 0.5.10.post1, H100 arc-runner-1-gpu-h100-gwjtg-*):
    • FAILED test_semantic_similarity[grpc] — AssertionError: Set 1, text 1: similarity 0.3342 not close to 1.0
    • FAILED test_relevance_scores[grpc] — AssertionError: Scores differ beyond tolerance
    • FAILED test_relevance_scores[http] — same
  • Both sglang 0.5.10 and 0.5.10.post1 produce identical divergence. The patch release does not touch the embedding path for this model family.
  • vllm embeddings on H100 passes cleanly (PR fix(ci): install ffmpeg OS libs for embeddings extras #1329 CI: e2e-1gpu-embeddings (vllm) / run SUCCESS).

0.3342 is an order of magnitude off 1.0, well beyond numerical-noise territory. Likely structural root cause (out of scope for this revert, to be investigated separately):

  1. Pooling strategy mismatch — E5-Mistral expects last-token pooling; sglang may be emitting mean-pooled output or vice versa.
  2. L2 normalization applied on one side only.
  3. Instruction-prefix handling ("Instruct: ...\nQuery: ") applied asymmetrically.

Since H100 alone does not fix this, the skip should be restored to unblock the sglang embeddings lane while the divergence is diagnosed by someone with context on sglang's embedding endpoint.

Changes

  • .github/workflows/pr-test-rust.yml: runner: 1-gpu-h100 → runner: 1-gpu
  • e2e_test/embeddings/test_correctness.py: re-add the skip_for_runtime("sglang", ...) decorator

Test plan

Follow-up

Worth opening a separate issue for the underlying sglang E5-Mistral embedding divergence so someone with sglang embedding-endpoint context can investigate pooling / normalization / prompt-formatting parity with the HF reference.

Summary by CodeRabbit

  • Tests
    • Embeddings correctness tests are now conditionally skipped for the sglang runtime.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions github-actions Bot added ci CI/CD configuration changes tests Test changes labels Apr 22, 2026
@coderabbitai

coderabbitai Bot commented Apr 22, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d4fac73e-0dde-4d74-b56e-3a585171d11a

📥 Commits

Reviewing files that changed from the base of the PR and between a1147a0 and 443d8fb.

📒 Files selected for processing (1)
  • e2e_test/embeddings/test_correctness.py

📝 Walkthrough

Walkthrough

A pytest marker is added to conditionally skip embedding correctness tests for the sglang runtime due to output divergence from HuggingFace reference results. No test logic, comparisons, or tolerance values are modified.

Changes

Cohort / File(s) Summary
Test Skip Marker
e2e_test/embeddings/test_correctness.py
Added skip_for_runtime pytest marker to TestEmbeddingCorrectness class to skip tests for sglang runtime where embedding outputs diverge from HuggingFace reference results.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Suggested reviewers

  • XinyueZhang369
  • key4ng

Poem

🐰 With one little skip, the tests now behave,
When sglang comes calling, we're no longer brave,
The embeddings dance, but they drift from the way,
So we hop right past them, and test another day! 🌙

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: restoring a skip_for_runtime marker on the sglang embedding correctness test, which directly aligns with the primary objective of reverting PR #1326.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch revert/1326-sglang-embedding-skip

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Clean revert — correctly restores the sglang skip marker and reverts the embedding runner to standard GPU. Changes are well-scoped (only embedding test job affected) and the commit message documents the evidence thoroughly.

PR #1326 removed `@pytest.mark.skip_for_runtime("sglang", reason="sglang
embedding output diverges from HF reference")` on the hypothesis that
H100 hardware would close the sglang vs HuggingFace reference gap for
`intfloat/e5-mistral-7b-instruct`. Empirically that is not the case:

PR #1329 CI (sglang 0.5.10 and 0.5.10.post1 on H100
arc-runner-1-gpu-h100-gwjtg-*):
  FAILED test_semantic_similarity[grpc]
    AssertionError: Set 1, text 1: similarity 0.3342 not close to 1.0
  FAILED test_relevance_scores[grpc,http]
    AssertionError: Scores differ beyond tolerance

0.3342 vs 1.0 is structural (pooling / normalization / instruction
prefix), not hardware — H100 alone does not close the gap.

Keep the H100 runner change from #1326 (embeddings lane still runs on
1-gpu-h100) so that when the underlying sglang divergence is fixed,
re-enabling this test is a one-line revert of this commit. Restore
only the skip decorator to unblock the sglang-embeddings CI lane in
the meantime.

Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Comment thread .github/workflows/pr-test-rust.yml Outdated
@slin1237
slin1237 force-pushed the revert/1326-sglang-embedding-skip branch from a1147a0 to 443d8fb Compare April 22, 2026 20:16
@github-actions github-actions Bot removed the ci CI/CD configuration changes label Apr 22, 2026
@slin1237 slin1237 changed the title Revert "test(embeddings): enable sglang embedding correctness on h100 (#1326)" test(embeddings): restore skip_for_runtime("sglang") on correctness test Apr 22, 2026
@slin1237
slin1237 merged commit 2fb15a5 into main Apr 22, 2026
33 of 36 checks passed
@slin1237
slin1237 deleted the revert/1326-sglang-embedding-skip branch April 22, 2026 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants