Repository navigation
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughCI scripts now detect Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good. Small, well-targeted CI fix — installs ffmpeg libs conditionally when sentence-transformers is an extra dep. The match pattern works for the current invocation, set -euo pipefail ensures failures aren't swallowed, and the comment explains the dependency chain clearly.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccd991ec80
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # which link against FFmpeg shared libs (libavformat/libavcodec/libavutil/ | ||
| # libswresample/libswscale). The runner image does not ship these, so | ||
| # install ffmpeg from apt before pip-installing the extra. | ||
| if [[ " $* " == *" sentence-transformers "* ]]; then |
There was a problem hiding this comment.
The ffmpeg install gate only matches an argument that is exactly sentence-transformers, so common pip forms like sentence-transformers==3.4.1 or sentence-transformers>=3 will skip the apt install and still hit the libtorchcodec/libav* runtime failure this commit is trying to prevent. This makes the fix brittle for normal dependency pinning in workflows that pass extra_deps with version constraints.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/ci_install_e2e_deps.sh`:
- Line 26: The current pattern match in scripts/ci_install_e2e_deps.sh using [[
" $* " == *" sentence-transformers "* ]] misses version-constrained specs like
sentence-transformers>=3; change the logic to iterate over the arguments (for
arg in "$@") and use a regex test (bash [[ $arg =~ ... ]]) to detect the package
name with optional version specifiers so any token starting with
"sentence-transformers" (e.g., sentence-transformers,
sentence-transformers==3.4.1, sentence-transformers>=3) triggers the FFmpeg
installation branch; update the if/check around that token (replace the current
if [[ " $* " == ... ]] condition) to use this per-argument regex test.
- Around line 28-29: The apt-get commands currently use sudo unconditionally
(the lines running "sudo apt-get update -qq" and "sudo apt-get install -y
--no-install-recommends ffmpeg"), which breaks in CI environments that run as
root or lack sudo; add a detection at the top of the script to set a SUDO
variable (e.g., SUDO='' when UID==0 or command -v sudo is missing, otherwise
SUDO='sudo') and replace direct "sudo apt-get ..." invocations with "${SUDO}
apt-get ..." so the same commands work in both root and non-root CI runners.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 194152e2-0b2e-4aec-a6c3-98734f4000a6
📒 Files selected for processing (1)
scripts/ci_install_e2e_deps.sh
| # which link against FFmpeg shared libs (libavformat/libavcodec/libavutil/ | ||
| # libswresample/libswscale). The runner image does not ship these, so | ||
| # install ffmpeg from apt before pip-installing the extra. | ||
| if [[ " $* " == *" sentence-transformers "* ]]; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how extra_deps values are passed to this script across workflows.
# Expected: identify whether any caller can pass version-constrained sentence-transformers specs.
rg -n -C3 'ci_install_e2e_deps\.sh|extra_deps|sentence-transformers' .github/workflowsRepository: lightseekorg/smg
Length of output: 7805
🏁 Script executed:
cat -n scripts/ci_install_e2e_deps.sh | head -50Repository: lightseekorg/smg
Length of output: 1547
Pattern match for sentence-transformers is too strict for version constraints.
The substring match [[ " $* " == *" sentence-transformers "* ]] fails to detect version-constrained specs like sentence-transformers>=3 or sentence-transformers==3.4.1. If such a spec is passed, FFmpeg installation will be skipped, reintroducing the import failure described in the inline comment.
The fix should iterate over arguments and use a regex to match the package name with optional version specifiers:
Proposed fix
- if [[ " $* " == *" sentence-transformers "* ]]; then
+ install_ffmpeg=false
+ for dep in "$@"; do
+ if [[ "$dep" =~ ^sentence-transformers($|[<>=!~\[].*) ]]; then
+ install_ffmpeg=true
+ break
+ fi
+ done
+ if [[ "$install_ffmpeg" == "true" ]]; then
echo "Installing ffmpeg (libav*) for torchcodec..."
sudo apt-get update -qq
sudo apt-get install -y --no-install-recommends ffmpeg
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci_install_e2e_deps.sh` at line 26, The current pattern match in
scripts/ci_install_e2e_deps.sh using [[ " $* " == *" sentence-transformers "* ]]
misses version-constrained specs like sentence-transformers>=3; change the logic
to iterate over the arguments (for arg in "$@") and use a regex test (bash [[
$arg =~ ... ]]) to detect the package name with optional version specifiers so
any token starting with "sentence-transformers" (e.g., sentence-transformers,
sentence-transformers==3.4.1, sentence-transformers>=3) triggers the FFmpeg
installation branch; update the if/check around that token (replace the current
if [[ " $* " == ... ]] condition) to use this per-argument regex test.
| sudo apt-get update -qq | ||
| sudo apt-get install -y --no-install-recommends ffmpeg |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where this script runs and whether containerized contexts are used.
# Expected: if called from container jobs, prefer sudo fallback to avoid command-not-found failures.
rg -n -C4 'container:|bash scripts/ci_install_e2e_deps\.sh' .github/workflowsRepository: lightseekorg/smg
Length of output: 4819
🏁 Script executed:
cat -n scripts/ci_install_e2e_deps.shRepository: lightseekorg/smg
Length of output: 1547
🏁 Script executed:
head -50 .github/workflows/pr-test-rust.yml | grep -A3 -B3 "runs-on\|container"Repository: lightseekorg/smg
Length of output: 157
🏁 Script executed:
rg "runs-on:|container:" .github/workflows/ | head -20Repository: lightseekorg/smg
Length of output: 1326
🏁 Script executed:
rg -A20 "bash scripts/ci_install_e2e_deps\.sh" .github/workflows/pr-test-rust.yml | head -40Repository: lightseekorg/smg
Length of output: 1438
🏁 Script executed:
rg -B5 "runs-on: k8s-runner" .github/workflows/Repository: lightseekorg/smg
Length of output: 7191
🏁 Script executed:
rg -B5 "runs-on: .*-gpu" .github/workflows/ | head -30Repository: lightseekorg/smg
Length of output: 1817
🏁 Script executed:
rg "ci_install_e2e_deps\.sh.*sentence-transformers|ci_install_e2e_deps\.sh.*\$.*extra_deps" .github/workflows/Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check what arguments are actually passed to the script in workflows
rg -A2 "bash scripts/ci_install_e2e_deps\.sh" .github/workflows/ | grep -A1 "ci_install_e2e_deps"Repository: lightseekorg/smg
Length of output: 1015
🏁 Script executed:
# Check what values are passed for matrix.variant.extra_deps and inputs.extra_deps
rg -B10 "matrix.variant.extra_deps|inputs.extra_deps" .github/workflows/nightly-benchmark.yml | grep -A5 -B5 "extra_deps:"Repository: lightseekorg/smg
Length of output: 42
🏁 Script executed:
# Check e2e-gpu-job.yml for inputs definition
rg -A5 "inputs:" .github/workflows/e2e-gpu-job.yml | head -30Repository: lightseekorg/smg
Length of output: 208
🏁 Script executed:
# Search for matrix definition and extra_deps in nightly-benchmark.yml
rg "matrix:" .github/workflows/nightly-benchmark.yml -A30 | head -50Repository: lightseekorg/smg
Length of output: 2942
🏁 Script executed:
# Also check the full e2e-gpu-job.yml inputs definition
cat .github/workflows/e2e-gpu-job.yml | grep -A50 "inputs:"Repository: lightseekorg/smg
Length of output: 1468
🏁 Script executed:
# Check if sentence-transformers is ever passed in any workflow
rg "sentence-transformers" .github/workflows/Repository: lightseekorg/smg
Length of output: 136
🏁 Script executed:
# Check full nightly-benchmark.yml variant definition to see all extra_deps values
rg -A2 "variant:" .github/workflows/nightly-benchmark.yml | grep -A2 "extra_deps"Repository: lightseekorg/smg
Length of output: 888
Conditionally use sudo for apt-get commands to support root-based CI environments.
Lines 28-29 will fail in CI runners that execute as root or lack sudo (common in Kubernetes-based or containerized CI environments). The script currently runs sudo apt-get unconditionally only when sentence-transformers is passed as a dependency—which does occur in pr-test-rust.yml.
Proposed fix
- sudo apt-get update -qq
- sudo apt-get install -y --no-install-recommends ffmpeg
+ APT_GET=(apt-get)
+ if command -v sudo >/dev/null 2>&1 && [ "$(id -u)" -ne 0 ]; then
+ APT_GET=(sudo apt-get)
+ fi
+ "${APT_GET[@]}" update -qq
+ "${APT_GET[@]}" install -y --no-install-recommends ffmpeg📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo apt-get update -qq | |
| sudo apt-get install -y --no-install-recommends ffmpeg | |
| APT_GET=(apt-get) | |
| if command -v sudo >/dev/null 2>&1 && [ "$(id -u)" -ne 0 ]; then | |
| APT_GET=(sudo apt-get) | |
| fi | |
| "${APT_GET[@]}" update -qq | |
| "${APT_GET[@]}" install -y --no-install-recommends ffmpeg |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/ci_install_e2e_deps.sh` around lines 28 - 29, The apt-get commands
currently use sudo unconditionally (the lines running "sudo apt-get update -qq"
and "sudo apt-get install -y --no-install-recommends ffmpeg"), which breaks in
CI environments that run as root or lack sudo; add a detection at the top of the
script to set a SUDO variable (e.g., SUDO='' when UID==0 or command -v sudo is
missing, otherwise SUDO='sudo') and replace direct "sudo apt-get ..."
invocations with "${SUDO} apt-get ..." so the same commands work in both root
and non-root CI runners.
Try the patch release to see if it resolves the embedding-output divergence from HF reference observed on PR #1329's sglang embeddings CI run (similarity 0.3342 vs expected ~1.0 for intfloat/e5-mistral-7b-instruct). 0.5.10.post1 was uploaded 2026-04-08 (three days after 0.5.10). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
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>
Try the patch release to see if it resolves the embedding-output divergence from HF reference observed on PR #1329's sglang embeddings CI run (similarity 0.3342 vs expected ~1.0 for intfloat/e5-mistral-7b-instruct). 0.5.10.post1 was uploaded 2026-04-08 (three days after 0.5.10). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
e39ede1 to
b9965df
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
scripts/ci_install_e2e_deps.sh (2)
28-29:⚠️ Potential issue | 🟠 MajorLines 28-29: unconditional
sudoreduces CI portability.Running
sudo apt-getunconditionally can fail on root-based or minimal runners withoutsudo.Proposed fix
- sudo apt-get update -qq - sudo apt-get install -y --no-install-recommends ffmpeg + APT_GET=(apt-get) + if command -v sudo >/dev/null 2>&1 && [ "$(id -u)" -ne 0 ]; then + APT_GET=(sudo apt-get) + fi + "${APT_GET[@]}" update -qq + "${APT_GET[@]}" install -y --no-install-recommends ffmpeg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci_install_e2e_deps.sh` around lines 28 - 29, The script currently runs "sudo apt-get update" and "sudo apt-get install -y --no-install-recommends ffmpeg" unconditionally, which breaks on runners without sudo or when already root; modify the CI install logic in scripts/ci_install_e2e_deps.sh to conditionally prepend sudo only when needed by checking for sudo availability and non-root execution (e.g., test command -v sudo and id -u != 0) before invoking apt-get, and fall back to running apt-get directly when running as root or when sudo is absent.
26-26:⚠️ Potential issue | 🟠 MajorLine 26: dependency detection is too strict for versioned specs.
[[ " $* " == *" sentence-transformers "* ]]misses arguments likesentence-transformers>=3, which can skip the FFmpeg install path.Proposed fix
- if [[ " $* " == *" sentence-transformers "* ]]; then + install_ffmpeg=false + for dep in "$@"; do + if [[ "$dep" =~ ^sentence-transformers($|[<>=!~\[].*) ]]; then + install_ffmpeg=true + break + fi + done + if [[ "$install_ffmpeg" == "true" ]]; then echo "Installing ffmpeg (libav*) for torchcodec..." sudo apt-get update -qq sudo apt-get install -y --no-install-recommends ffmpeg fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci_install_e2e_deps.sh` at line 26, The if condition that checks for the dependency in scripts/ci_install_e2e_deps.sh is too strict: the pattern [[ " $* " == *" sentence-transformers "* ]] requires spaces around the token and will miss versioned specs like sentence-transformers>=3. Update the conditional that contains that exact pattern so it matches any argument containing the substring "sentence-transformers" (e.g., remove the surrounding space constraint or use a regex match such as [[ " $* " == *sentence-transformers* ]] or [[ $* =~ sentence-transformers ]]) so versioned specifications are detected and the FFmpeg install path is not skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/ci_install_e2e_deps.sh`:
- Around line 28-29: The script currently runs "sudo apt-get update" and "sudo
apt-get install -y --no-install-recommends ffmpeg" unconditionally, which breaks
on runners without sudo or when already root; modify the CI install logic in
scripts/ci_install_e2e_deps.sh to conditionally prepend sudo only when needed by
checking for sudo availability and non-root execution (e.g., test command -v
sudo and id -u != 0) before invoking apt-get, and fall back to running apt-get
directly when running as root or when sudo is absent.
- Line 26: The if condition that checks for the dependency in
scripts/ci_install_e2e_deps.sh is too strict: the pattern [[ " $* " == *"
sentence-transformers "* ]] requires spaces around the token and will miss
versioned specs like sentence-transformers>=3. Update the conditional that
contains that exact pattern so it matches any argument containing the substring
"sentence-transformers" (e.g., remove the surrounding space constraint or use a
regex match such as [[ " $* " == *sentence-transformers* ]] or [[ $* =~
sentence-transformers ]]) so versioned specifications are detected and the
FFmpeg install path is not skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b7507db-af0e-4386-8ac1-2b97efaced41
📒 Files selected for processing (2)
scripts/ci_install_e2e_deps.shscripts/ci_install_sglang.sh
sentence-transformers >= 3.x top-level imports torchcodec via
modality_types, which dlopens libtorchcodec_core{4..8}.so linked
against FFmpeg shared libs (libavformat/libavcodec/libavutil/
libswresample/libswscale). The runner image does not ship these.
PR #1326 removed the `skip_for_runtime("sglang", ...)` decorator on
test_correctness.py, so from that merge onward every sglang
embeddings run imports torchcodec at fixture setup and errors with:
RuntimeError: Could not load libtorchcodec. Likely causes:
1. FFmpeg is not properly installed ...
Previously the test was skipped on sglang, which hid the missing
libav* on the runner image.
Gated strictly on the embeddings matrix via the `sentence-transformers`
marker in extra_deps (pr-test-rust.yml:511). Other CI lanes see no
apt traffic.
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Try the patch release to see if it resolves the embedding-output divergence from HF reference observed on PR #1329's sglang embeddings CI run (similarity 0.3342 vs expected ~1.0 for intfloat/e5-mistral-7b-instruct). 0.5.10.post1 was uploaded 2026-04-08 (three days after 0.5.10). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
b9965df to
67d9ebb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/ci_install_e2e_deps.sh (1)
26-29:⚠️ Potential issue | 🟠 MajorHarden ffmpeg gating and privilege handling in the same block.
Line [26] only matches the exact token
sentence-transformers; versioned specs (for examplesentence-transformers>=3) bypass the ffmpeg install.
Lines [28]-[29] assumesudois always available and needed, which is fragile across CI runner modes.Proposed fix
- if [[ " $* " == *" sentence-transformers "* ]]; then + install_ffmpeg=false + for dep in "$@"; do + if [[ "$dep" =~ ^sentence-transformers($|[<>=!~\[].*) ]]; then + install_ffmpeg=true + break + fi + done + if [[ "$install_ffmpeg" == "true" ]]; then echo "Installing ffmpeg (libav*) for torchcodec..." - sudo apt-get update -qq - sudo apt-get install -y --no-install-recommends ffmpeg + APT_GET=(apt-get) + if command -v sudo >/dev/null 2>&1 && [ "$(id -u)" -ne 0 ]; then + APT_GET=(sudo apt-get) + fi + "${APT_GET[@]}" update -qq + "${APT_GET[@]}" install -y --no-install-recommends ffmpeg fi#!/bin/bash set -euo pipefail # Verify the current match behavior used on Line 26. for dep in \ "sentence-transformers" \ "sentence-transformers>=3" \ "sentence-transformers==3.4.1"; do if [[ " $dep " == *" sentence-transformers "* ]]; then echo "MATCH: $dep" else echo "MISS : $dep" fi done # Verify runner privilege assumptions relevant to Lines 28-29. echo "uid=$(id -u)" if command -v sudo >/dev/null 2>&1; then echo "sudo=present" else echo "sudo=missing" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci_install_e2e_deps.sh` around lines 26 - 29, The gating condition [[ " $* " == *" sentence-transformers "* ]] only matches the exact token and misses versioned specs (e.g., sentence-transformers>=3), and the ffmpeg install block assumes sudo is available; update the check to robustly detect any argument containing the substring "sentence-transformers" (e.g., using a pattern match against "$*"/loop over "$@" or use =~) and only run the ffmpeg install when that match succeeds, and change the install commands (the ffmpeg install block that calls apt-get and sudo) to detect whether sudo exists and whether the script is running as root (uid 0) and conditionally prefix apt-get with sudo or run it directly so installations work across CI runners without assuming sudo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/ci_install_e2e_deps.sh`:
- Around line 26-29: The gating condition [[ " $* " == *" sentence-transformers
"* ]] only matches the exact token and misses versioned specs (e.g.,
sentence-transformers>=3), and the ffmpeg install block assumes sudo is
available; update the check to robustly detect any argument containing the
substring "sentence-transformers" (e.g., using a pattern match against "$*"/loop
over "$@" or use =~) and only run the ffmpeg install when that match succeeds,
and change the install commands (the ffmpeg install block that calls apt-get and
sudo) to detect whether sudo exists and whether the script is running as root
(uid 0) and conditionally prefix apt-get with sudo or run it directly so
installations work across CI runners without assuming sudo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40999e6a-8384-4c0f-8438-4b5339ba88a2
📒 Files selected for processing (2)
scripts/ci_install_e2e_deps.shscripts/ci_install_sglang.sh
2 participants
Summary
e2e-1gpu-embeddings (sglang)and(vllm)fail at fixture setup with:Root cause:
sentence-transformers >= 3.xtop-level importstorchcodecviamodality_types.torchcodecdlopenslibtorchcodec_core{4..8}.so, which link against FFmpeg shared libs (libavformat,libavcodec,libavutil,libswresample,libswscale). The runner image does not ship these.Why this surfaced now
PR #1326 removed
@pytest.mark.skip_for_runtime("sglang", reason="sglang embedding output diverges from HF reference")frome2e_test/embeddings/test_correctness.py:212. First failure (18:32:58 UTC) arrived 94 seconds after #1326 merged (18:31:24 UTC). Prior to #1326, the test was skipped on sglang, sotorchcodecwas never imported at test time and the missinglibav*was invisible. Apt package lists for H100 pods are bit-identical pre- and post-move (145 packages, zero diff) — the container image is unchanged; the latent dep gap was simply exposed.Change
Gate an
apt-get install -y ffmpegonsentence-transformersappearing inextra_deps. The embeddings matrix is the only lane that passes this extra (pr-test-rust.yml:511), so no other CI lane is affected.ffmpegon Ubuntu 24.04 (noble) pulls inlibavformat61 libavcodec61 libavutil59 libswresample5 libswscale8as direct deps, satisfying all torchcodec.solookups.Test plan
bash -n scripts/ci_install_e2e_deps.sh— syntax OKtest_correctness.pyfixture that currently errors withCould not load libtorchcodecshould import successfully and proceed to model loading.sentence-transformersin itsextra_deps, so the gated apt install is a no-op elsewhere.Alternatives considered
Summary by CodeRabbit