Skip to content

fix(test): skip HF-downloading embedding nodes in default CI lane#1234

Open
meetp06 wants to merge 1 commit into
rocketride-org:developfrom
meetp06:fix/RR-1120-skip-hf-embedding-tests
Open

fix(test): skip HF-downloading embedding nodes in default CI lane#1234
meetp06 wants to merge 1 commit into
rocketride-org:developfrom
meetp06:fix/RR-1120-skip-hf-embedding-tests

Conversation

@meetp06

@meetp06 meetp06 commented Jun 11, 2026

Copy link
Copy Markdown

Summary

Two test/test_dynamic.py::TestDynamicNodes::test_node_cases parametrizations download model weights from huggingface.co at test time:

  • embedding_video:services:openai-patch16 (CLIP)
  • embedding_transformer:services:miniLM (sentence-transformers)

When the HF hub is unreachable or rate-limited they raise RuntimeError: We couldn't connect to 'https://huggingface.co' ... and turn the only required check (ci-ok) red — on PRs that have nothing to do with embeddings. Per #1120 this has hit a docs-only PR, the Baidu Qianfan #1006 run, and the 2026-06-04 release prep, and it also blocks develop→stage promotion and the nightly prerelease gate.

Fix

conftest.py already maintains a skip_nodes set that excludes heavy / model-downloading nodes from the default dynamic-test lane — including embedding_image, which is the exact same category. embedding_transformer and embedding_video were simply missing from it. This adds them, with a comment explaining why.

'embedding_image',
# Download model weights from huggingface.co at test time ... (RR-1120)
'embedding_transformer',  # sentence-transformers (miniLM)
'embedding_video',        # CLIP (openai-patch16)

This is the issue's option 3 (gate so they don't run in the default lane), implemented via the mechanism the repo already uses — no new markers, no CI-image changes.

Behavior

  • Default lane (what CI runs via ./builder test) no longer touches the network for these nodes → ci-ok becomes deterministic.
  • They remain runnable on demand, exactly like the other skipped nodes:
    ROCKETRIDE_INCLUDE_SKIP=embedding_transformer,embedding_video pytest nodes/test/test_dynamic.py -v -k 'embedding_transformer or embedding_video'
    
  • Contract tests and every other node's tests are unaffected.

Trade-off

Like the existing skipped nodes (embedding_image, ocr, anonymize, …), these two are no longer smoke-tested in CI — a deliberate, precedent-matching trade of a small coverage loss for a non-flaky required check. A heavier alternative (pre-cache weights + HF_HUB_OFFLINE=1) would keep coverage but needs CI-runner-image changes; out of scope here.

Verification

python -m py_compile nodes/test/conftest.py passes; the two skipped node names match the failing parametrizations named in the issue (embedding_videoopenai-patch16, embedding_transformerminiLM), each of whose default test block runs only that single network-dependent profile.

Closes #1120

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Tests
    • Updated test configuration to skip certain embedding-related nodes during test execution.

…cketride-org#1120)

embedding_transformer (miniLM) and embedding_video (openai-patch16)
download model weights from huggingface.co while test_dynamic.py runs, so
when the HF hub is unreachable or rate-limited they fail and turn the only
required check (ci-ok) red — on PRs unrelated to embeddings. This has hit
docs-only PRs, the Baidu Qianfan rocketride-org#1006 run, and the 2026-06-04 release prep.

Add both to the existing skip_nodes set in conftest.py, matching the
embedding_image precedent (same heavy-model category). They remain runnable
on demand via ROCKETRIDE_INCLUDE_SKIP=embedding_transformer,embedding_video.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added the module:nodes Python pipeline nodes label Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eed8e720-410b-4731-a79a-19712059b0fa

📥 Commits

Reviewing files that changed from the base of the PR and between 1995c64 and 7cab78e.

📒 Files selected for processing (1)
  • nodes/test/conftest.py

📝 Walkthrough

Walkthrough

Updated the pytest dynamic test generation configuration to skip embedding_transformer and embedding_video nodes by default. These nodes download model weights from huggingface.co at test time, causing flaky CI failures on unrelated PRs. The skip list and inline comments were expanded to document this behavior.

Changes

Test Configuration

Layer / File(s) Summary
Skip embedding nodes in default test suite
nodes/test/conftest.py
The pytest_generate_tests skip_nodes filter now excludes embedding_transformer and embedding_video, with updated comments explaining these nodes download model weights from huggingface.co at test time (and are skipped unless explicitly opted in).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐇 Embedding nodes were slow and flaky on CI,
Downloading weights from HF way up high,
Now they're skipped by default, what a delight,
CI stays green throughout the night! 🌙✨

🚥 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: skipping HF-downloading embedding nodes in the default CI lane by adding them to the skip_nodes filter.
Linked Issues check ✅ Passed The code changes directly implement the selected resolution from issue #1120: gating embedding_transformer and embedding_video test cases by adding them to the skip_nodes set to avoid flaky network-dependent test failures in the default CI lane.
Out of Scope Changes check ✅ Passed All changes are focused on the skip_nodes filter in conftest.py; no out-of-scope modifications to unrelated code or test logic are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

Copy link
Copy Markdown
🤖 Internal: Discord sync marker

Auto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete.

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

Labels

module:nodes Python pipeline nodes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: embedding node tests hit live huggingface.co → flaky red CI on unrelated PRs

1 participant