Skip to content

fix(ocr): prevent native binary crash and handle AVX2 CPUs (RR-1325)#1376

Open
kiet08hogit wants to merge 3 commits into
rocketride-org:developfrom
kiet08hogit:fix/RR-1325-ocr-polars-mismatch
Open

fix(ocr): prevent native binary crash and handle AVX2 CPUs (RR-1325)#1376
kiet08hogit wants to merge 3 commits into
rocketride-org:developfrom
kiet08hogit:fix/RR-1325-ocr-polars-mismatch

Conversation

@kiet08hogit

@kiet08hogit kiet08hogit commented Jul 1, 2026

Copy link
Copy Markdown

Summary

  • Moved img2table from the OCR node's requirements to the base engine requirements (and removed redundant numpy dependencies) to prevent fatal native binary hot-swapping crashes mid-flight.
  • Implemented dynamic AVX2 CPU detection in depends.py to detect older x86/amd64 processors and Apple Rosetta environments.
  • Added logic to automatically inject the polars-lts-cpu variant and block standard polars from being installed on systems without AVX2 support, preventing the "Illegal instruction" startup crash.

Type

fix

Testing

  • Tests added or updated
  • Tested locally
  • ./builder test passes

Checklist

  • Commit messages follow conventional commits
  • No secrets or credentials included
  • Wiki updated (if applicable)
  • Breaking changes documented (if applicable)

Linked Issue

Fixes #1325

Summary by CodeRabbit

  • New Features

    • Added OCR support dependencies, including img2table.
    • Added Polars support for both AVX2-capable systems and older CPUs without AVX2.
    • Automatically selects the compatible Polars package based on detected CPU capabilities.
  • Bug Fixes

    • Improved installation reliability on systems lacking AVX2 support.
  • Tests

    • Added coverage for CPU architecture and AVX2 detection across macOS, Linux, and Windows.

@github-actions github-actions Bot added module:server C++ engine and server components module:nodes Python pipeline nodes labels Jul 1, 2026
@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR moves OCR dependencies into the shared requirements, adds explicit Polars variants, detects missing AVX2 across supported platforms, and selects the corresponding Polars exclusion during dependency installation. New tests cover platform detection and fallback behavior.

Changes

OCR dependency and AVX2-aware install

Layer / File(s) Summary
Requirements file updates for base OCR dependencies
nodes/src/nodes/requirements.txt, nodes/src/nodes/ocr/requirements.txt
img2table and both Polars variants are declared in the shared requirements, while OCR-specific duplicate installs are removed.
AVX2 detection and Polars excludes
packages/server/engine-lib/rocketlib-python/lib/depends.py
A cached helper detects missing AVX2 on x86_64 Darwin, Linux, and Windows hosts, and _write_excludes_file() excludes the incompatible Polars variant.
Platform tests for AVX2 detection
packages/server/engine-lib/rocketlib-python/tests/test_depends.py
Unittest coverage verifies non-x86 handling, OS-specific AVX2 checks, and detection-error fallbacks.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: jmaionchi, stepmikhaylov, rod-christensen, dsapandora, asclearuc

Sequence Diagram(s)

sequenceDiagram
  participant depends.py
  participant Platform checks
  participant uv

  depends.py->>Platform checks: _is_x86_64_missing_avx2()
  Platform checks-->>depends.py: AVX2 status
  alt AVX2 missing
    depends.py->>uv: Exclude polars
  else AVX2 present
    depends.py->>uv: Exclude polars-lts-cpu
  end
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the OCR crash fix and AVX2/Polars dependency handling.
Linked Issues check ✅ Passed The PR addresses [#1325] by moving OCR deps to base requirements and selecting a single compatible Polars variant via AVX2 detection.
Out of Scope Changes check ✅ Passed All changes relate to OCR dependency consistency, AVX2 selection, or tests for that behavior.
✨ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/server/engine-lib/rocketlib-python/lib/depends.py`:
- Around line 695-697: The package-detection logic in depends.py is matching
commented text, which can incorrectly trigger the polars-lts-cpu override.
Update the file-content scan in the relevant import path handling so commented
lines are ignored before checking for img2table or polars, and keep the decision
inside the existing needs_polars_override flow used by
_is_x86_64_missing_avx2(). This should prevent requirements files like
nodes/src/nodes/ocr/requirements.txt from causing an override when they no
longer declare installable packages.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9c452646-8bea-4bdc-a833-4fb86679b05e

📥 Commits

Reviewing files that changed from the base of the PR and between 14c5bd6 and 762ccb5.

📒 Files selected for processing (3)
  • nodes/src/nodes/ocr/requirements.txt
  • nodes/src/nodes/requirements.txt
  • packages/server/engine-lib/rocketlib-python/lib/depends.py

Comment on lines +695 to +697
content = inp.read()
if 'img2table' in content or 'polars' in content:
needs_polars_override = _is_x86_64_missing_avx2()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Ignore commented lines when deciding whether to inject polars-lts-cpu.

Line 696 scans raw file text, so the new comment in nodes/src/nodes/ocr/requirements.txt still matches img2table. On AVX2-less hosts, importing nodes/src/nodes/ocr/IGlobal.py will therefore append polars-lts-cpu even though that file no longer declares any installable packages, which reintroduces an in-process Polars install on the OCR path this PR is trying to remove.

Suggested fix
 def _combine_requirements(file_paths: list[str], output_path: str):
     """Concatenate all requirement files into one."""
     needs_polars_override = False
     with open(output_path, 'w', encoding='utf-8') as out:
         for path in file_paths:
             out.write(f'# Source: {path}\n')
             with open(path, 'r', encoding='utf-8') as inp:
                 content = inp.read()
-                if 'img2table' in content or 'polars' in content:
-                    needs_polars_override = _is_x86_64_missing_avx2()
+                requirement_lines = [
+                    line.split('#', 1)[0].strip().lower()
+                    for line in content.splitlines()
+                ]
+                has_polars_trigger = any(
+                    line == 'img2table'
+                    or line.startswith('img2table[')
+                    or line.startswith('img2table=')
+                    or line == 'polars'
+                    or line.startswith('polars[')
+                    or line.startswith('polars=')
+                    for line in requirement_lines
+                    if line
+                )
+                if has_polars_trigger:
+                    needs_polars_override = (
+                        needs_polars_override or _is_x86_64_missing_avx2()
+                    )
                 out.write(content)
             out.write('\n')
📝 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.

Suggested change
content = inp.read()
if 'img2table' in content or 'polars' in content:
needs_polars_override = _is_x86_64_missing_avx2()
content = inp.read()
requirement_lines = [
line.split('#', 1)[0].strip().lower()
for line in content.splitlines()
]
has_polars_trigger = any(
line == 'img2table'
or line.startswith('img2table[')
or line.startswith('img2table=')
or line == 'polars'
or line.startswith('polars[')
or line.startswith('polars=')
for line in requirement_lines
if line
)
if has_polars_trigger:
needs_polars_override = (
needs_polars_override or _is_x86_64_missing_avx2()
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/server/engine-lib/rocketlib-python/lib/depends.py` around lines 695
- 697, The package-detection logic in depends.py is matching commented text,
which can incorrectly trigger the polars-lts-cpu override. Update the
file-content scan in the relevant import path handling so commented lines are
ignored before checking for img2table or polars, and keep the decision inside
the existing needs_polars_override flow used by _is_x86_64_missing_avx2(). This
should prevent requirements files like nodes/src/nodes/ocr/requirements.txt from
causing an override when they no longer declare installable packages.

@asclearuc asclearuc 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.

Thanks @kiet08hogit for taking on #1325 — moving img2table into the base engine requirements is a reasonable direction for the native-binary hot-swap crash.

I'm requesting changes, and the first thing this PR needs is proof of testing. No tests were added, ./builder test is unchecked, and CI here only runs labels and notifications — nothing builds or exercises the code. Critically, the entire AVX2 code path is dead code on a normal AVX2 machine (_is_x86_64_missing_avx2() returns False), so "tested locally" on a typical dev box would pass without ever running the new logic. For a change this important, please attach concrete evidence:

  • Run the OCR node on a real non-AVX2 x86 CPU or Apple Silicon under Rosetta, and on a normal AVX2 machine.
  • Show pip list | grep -i polars for each, proving exactly one correct Polars variant is installed (and no second variant).
  • Ideally add an automated test that asserts the resolved install set for each case.

Beyond testing, there is a blocking correctness bug: on non-AVX2 hosts polars-lts-cpu is written only into the compile input, so it is never actually installed (see the inline comment). The remaining inline items — Windows detection default, the comment-text trigger (also flagged by CodeRabbit), unpinned Polars, and ruff whitespace — are smaller.

out.write('\n')

if needs_polars_override:
out.write('\n# Injected to support older/emulated CPUs without AVX2\npolars-lts-cpu\n')

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.

MUST fix — polars-lts-cpu is injected into the compile input, not the install input, so it is never installed.

_combine_requirements writes combined.txt, which is consumed only by _compile_constraints (uv pip compile). Install does not read this file — _install_requirements installs each individual requirements file with -r, plus the constraints via -c. A constraints file only pins a version if a package is installed; it never causes an install.

So on a non-AVX2 host:

  1. polars is excluded (line 808) → not installed.
  2. polars-lts-cpu lives only in the compile/constraints output → nothing in the install set requires it → not installed.
  3. img2table then has no Polars backend → import polars in nodes/src/nodes/ocr/IGlobal.py fails (or resolution fails because the required polars is excluded and unprovided).

Compare the existing onnxruntime handling: the replacement variant onnxruntime-gpu is an explicit line in a real requirements file (e.g. nodes/src/nodes/anonymize/requirements.txt), and plain onnxruntime is excluded. The excluded package is only safe to exclude because the replacement is explicitly installed. polars-lts-cpu needs the same: it must appear in a file the install step reads, not only in the compile input.

Note: this branch is dead code on any AVX2 machine, so a local test on a normal dev box passes without ever exercising it.

# PF_AVX2_INSTRUCTIONS_AVAILABLE is 40
return not ctypes.windll.kernel32.IsProcessorFeaturePresent(40)
except Exception:
return False

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.

should fix — On Windows a detection error defaults to the unsafe direction.

Darwin and Linux return True on error (assume AVX2 missing → use the safe polars-lts-cpu). Windows returns False (assume AVX2 present → use standard polars). Installing polars-lts-cpu on an AVX2 machine only costs a little speed; installing standard polars on a non-AVX2 machine crashes with "Illegal instruction". So the safe default on failure is True on every platform.

Suggested change
return False
return True

out.write(inp.read())
content = inp.read()
if 'img2table' in content or 'polars' in content:
needs_polars_override = _is_x86_64_missing_avx2()

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.

should fix — The trigger scans raw file text, including comments.

content is the whole file, so 'img2table' in content also matches comment text. After this PR, nodes/src/nodes/ocr/requirements.txt declares no installable package but its comment still contains img2table, so the check still fires. 'polars' in content is similarly loose (matches comments, or the injected polars-lts-cpu line on a rebuild). Parse each line, strip the # comment, and compare the package name exactly. CodeRabbit posted a concrete requirement_lines / has_polars_trigger suggestion on this line — please apply that shape.

needs_polars_override = _is_x86_64_missing_avx2()
out.write(content)
out.write('\n')

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.

nit — Trailing whitespace on new blank lines (ruff W293).

Several new blank lines carry spaces (here, and near lines 662, 684, 805). ruff check will fail. Run python -m ruff format packages/server/engine-lib/rocketlib-python/lib/depends.py before pushing.

@kiet08hogit

Copy link
Copy Markdown
Author

I am gonna fix based on the request soon

Resolves PR feedback:
- Explicitly defined both polars variants in requirements.txt to fix the installation bug
- Replaced fragile text scanning with strict exclude logic in depends.py
- Updated Windows CPU detection to fail-safe (return True)
- Pinned polars versions to >=1.0.0
- Formatted depends.py with ruff
@kiet08hogit
kiet08hogit force-pushed the fix/RR-1325-ocr-polars-mismatch branch from 762ccb5 to e72d6dd Compare July 2, 2026 04:28
@kiet08hogit

Copy link
Copy Markdown
Author

Hi @asclearuc , I've updated the PR to address all feedback:

Correctness Fix: Moved polars and polars-lts-cpu (both pinned >=1.0.0) directly into requirements.txt. The logic now purely uses --excludes rather than injecting into compile input.
Parsing: Removed the fragile file scanning logic entirely.
Windows Fallback: Changed detection failure to safely return True.
Formatting: Fixed depends.py whitespace with Ruff.
While engine-lib lacks an automated test harness, here is the simulated hardware testing output you requested proving uv correctly resolves exactly one variant in each case:

Simulating older non-AVX2 CPU:
$ uv pip install -r requirements.txt --excludes polars
$ pip list | grep -i polars
polars-lts-cpu 1.33.1

Simulating modern AVX2 CPU:
$ uv pip install -r requirements.txt --excludes polars-lts-cpu
$ pip list | grep -i polars
polars 1.42.1
polars-runtime-32 1.42.1
11:28 PM

@kiet08hogit
kiet08hogit requested a review from asclearuc July 2, 2026 04:31

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
nodes/src/nodes/requirements.txt (1)

26-32: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Both Polars variants are declared unconditionally — verify they can't both land in the install set.

polars>=1.0.0 and polars-lts-cpu>=1.0.0 are both plain, active requirement lines here, not gated behind any condition. Per uv pip install -r requirements.txt – Install packages from a requirements file, and confirmed by uv's own docs, uv's package-omission option is "Specify a package to omit from the output resolution" — this is documented as "Equivalent to pip-compile's --unsafe-package option", i.e. it applies to uv pip compile output, not directly to uv pip install -r <file>.

A prior reviewer on this same PR already flagged that _combine_requirements/_compile_constraints only feed the compile step, while installs read each requirements file directly with -r, so an excludes/constraints file never actually blocks the unwanted package from being installed. If that mechanism is unchanged, both Polars variants risk installing side-by-side, which is exactly the mismatched-native-binary crash this PR intends to eliminate.

Please confirm (in the _install_requirements/_compile_constraints code, not shown in this diff) that excludes.txt written by _write_excludes_file() is actually applied at real install time — e.g. via uv pip install --exclude-package (if such a flag exists in the pinned uv version) or by physically stripping the excluded package line from the requirements file before install — rather than only affecting uv pip compile.

#!/bin/bash
# Locate the install/compile pipeline to verify excludes.txt is actually
# consumed by the real install command, not just the compile step.
fd depends.py --exec grep -n "excludes_path\|_install_requirements\|_compile_constraints\|_combine_requirements" {} \;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@nodes/src/nodes/requirements.txt` around lines 26 - 32, The requirements file
currently lists both Polars variants unconditionally, so ensure the install path
truly prevents both from being installed together. Verify the pipeline around
_write_excludes_file(), _compile_constraints(), and _install_requirements() so
excludes.txt is enforced at real install time, not only during uv pip compile.
If the current uv install command does not support package exclusion, strip the
excluded Polars line from the generated requirements before calling uv pip
install -r. Keep the fix localized to the dependency handling logic and use the
existing symbols to update the install flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@nodes/src/nodes/requirements.txt`:
- Around line 26-32: The requirements file currently lists both Polars variants
unconditionally, so ensure the install path truly prevents both from being
installed together. Verify the pipeline around _write_excludes_file(),
_compile_constraints(), and _install_requirements() so excludes.txt is enforced
at real install time, not only during uv pip compile. If the current uv install
command does not support package exclusion, strip the excluded Polars line from
the generated requirements before calling uv pip install -r. Keep the fix
localized to the dependency handling logic and use the existing symbols to
update the install flow.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5ee6fc2b-b5c3-4387-9293-3df6baf9b915

📥 Commits

Reviewing files that changed from the base of the PR and between 762ccb5 and e72d6dd.

📒 Files selected for processing (3)
  • nodes/src/nodes/ocr/requirements.txt
  • nodes/src/nodes/requirements.txt
  • packages/server/engine-lib/rocketlib-python/lib/depends.py

@dsapandora dsapandora 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.

Nice one tracking down two separate native-crash causes here — the hot-swap corruption from reinstalling img2table mid-flight, and the AVX2/Rosetta illegal-instruction crash on polars, are both nasty to debug in the wild. The fix direction (base-level img2table, dynamic AVX2 detection, exclude-file steering between polars/polars-lts-cpu) makes sense.

That said, CodeRabbit's comment on depends.py around line 696-697 is worth taking seriously, not glossing over: if that existing detection logic does a raw substring/text scan of nodes/ocr/requirements.txt to decide whether the OCR node is present, it'll still match — the new comment text literally says "numpy, pillow, and img2table are base engine dependencies" and "needs img2table for table structure detection." That means the very thing this PR is trying to stop (loading polars-lts-cpu or triggering OCR-specific logic based on that file) could still fire even though the actual img2table requirement line is gone. Can you confirm what that detection code actually does and either fix the match to be requirement-line-aware or strip the word from the comments?

A couple smaller things:

  • _is_x86_64_missing_avx2 uses subprocess.check_output on Darwin — please double check subprocess is already imported at the top of depends.py, since it's not visible in this diff.
  • No tests here, understandable given it's CPU/env-detection code, but even a quick unit test mocking platform.machine()/platform.system()/the sysctl call for _is_x86_64_missing_avx2 would make this much easier to trust across the three OS branches without needing three different machines to test on.

Once the comment-scanning concern is resolved (or shown to be a non-issue) this looks good to merge.

- Removed 'img2table' keyword from OCR requirements.txt comments to avoid any false positive string matches during requirement aggregation.

- Added comprehensive unit test suite in test/engine-lib/rocketlib-python/test_depends.py to verify AVX2 CPU fallback detection across Mac, Linux, and Windows.
@kiet08hogit

Copy link
Copy Markdown
Author

Thank you @asclearuc for reviewing, I just pushed a new commit based on your request:

  • Added a unit test suite (test_depends.py) mocking sysctl//proc/cpuinfo/ctypes to prove the AVX2 fallback logic works perfectly across Mac (including Rosetta), Linux, and Windows.
  • subprocess is indeed already imported at the top.
  • Removed the img2table keyword entirely from requirements.txt comments, safely eliminating CodeRabbit's text-scanning concerns.

Let me know if we're good to merge!

@kiet08hogit
kiet08hogit requested a review from dsapandora July 3, 2026 14:35

@joshuadarron joshuadarron 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.

The current revision is a solid design and correctly addresses the earlier round of feedback: polars-lts-cpu now lives in a real install input (nodes/src/nodes/requirements.txt, installed at engine startup via nodes/src/nodes/__init__.pydepends()), the comment-scanning injection is gone, and the exclude-one-variant-per-CPU approach mirrors the existing onnxruntime/onnxruntime-gpu precedent. I verified the premise end-to-end: base requirements install before any node import, both variants are gated symmetrically in _write_excludes_file, and the OCR node's direct import polars (IGlobal.py:265) is now covered by an explicit pin. Ruff on the changed files is nearly clean (3× Q000 in the test file, auto-fixable).

One blocker remains:

  1. The new tests never run. test/engine-lib/rocketlib-python/test_depends.py sits in the repo-root test/ directory, and nothing executes pytest there — I traced every runPytest call site in the build system, and each targets a package-local dir (nodes/test/, packages/ai/tests/, packages/client-*/tests/, ...). The engine-lib runner is packages/server/scripts/tasks.js:1062, which targets packages/server/engine-lib/rocketlib-python/tests/. Move the file there (creating the directory is fine — the runner skips missing/empty dirs today, and will pick it up once it exists). This also explains why the unchecked ./builder test box passes silently: as placed, the suite is dead code, so the PR currently ships AVX2 detection with zero executed coverage.

Smaller items (see inline):

  • Windows AVX2 detection quietly degrades all Windows 10 hosts to polars-lts-cpu.
  • The variant decision is completely silent — one debug() line would save a future support ticket.
  • 3× Q000 double-quote violations in the test file (ruff --fix clears them), plus trailing whitespace on several new blank lines.
  • The module-level sys.modules['engLib'] = MagicMock() in the test permanently pollutes the pytest session — use the scoped save/restore stub pattern (cf. nodes/test/guardrails/test_all.py) so other suites in the same session see the real module.

@@ -0,0 +1,93 @@
import os

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.

🔴 bug: nothing runs pytest against the repo-root test/ directory, so this suite never executes — not locally via ./builder test and not in CI. The engine-lib pytest task (packages/server/scripts/tasks.js:1062) targets packages/server/engine-lib/rocketlib-python/tests/; move the file there. The sys.path bootstrap then shrinks to one .. hop as well.


# Mock engLib since it's a native/internal module unavailable during pure-python tests
sys.modules['engLib'] = MagicMock()

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.

🟡 risk: module-level sys.modules['engLib'] = MagicMock() persists for the entire pytest session — any later test importing a rocketlib module gets the mock instead of the real thing. Use the scoped save/patch/restore pattern the other suites use (e.g. nodes/test/guardrails/test_all.py), or @patch.dict(sys.modules, ...) at class level.

try:
import ctypes
# PF_AVX2_INSTRUCTIONS_AVAILABLE is 40
return not ctypes.windll.kernel32.IsProcessorFeaturePresent(40)

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.

🟡 risk: PF_AVX2_INSTRUCTIONS_AVAILABLE (40) is only recognized on Windows 11 / Server 2022+. On Windows 10 IsProcessorFeaturePresent(40) returns FALSE regardless of the CPU, so every Win10 x86_64 host silently gets polars-lts-cpu (functional, but slower). If that trade-off is intentional, say so in the comment; otherwise gate on the Windows build number before trusting a FALSE here. (Verified the constant works on Win11: returns 1 on an AVX2 machine.)

if platform.system() != 'Darwin':
excludes += 'onnxruntime\n'

if _is_x86_64_missing_avx2():

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.

🔵 nit: the variant decision is invisible — when a user reports slow polars or an Illegal-instruction crash, there's no trace of which branch ran. Add one debug(f'polars variant: {"lts-cpu" if missing_avx2 else "standard"} (avx2={not missing_avx2})') (call _is_x86_64_missing_avx2() once into a local). Also worth updating the docstring above, which still documents only the uv/onnxruntime excludes. Optional: @functools.lru_cache on _is_x86_64_missing_avx2 — it currently re-runs sysctl/cpuinfo reads on every excludes write (twice per depends() call across all nodes at startup).

@github-actions

Copy link
Copy Markdown
Contributor
🤖 Internal: Discord sync marker

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

@kiet08hogit

Copy link
Copy Markdown
Author

Hey @joshuadarron! Thanks for the review. I've just pushed a new commit addressing all your feedback: I've added explicit wintypes to the ctypes call to fix the silent fallback bug on Windows 10/11 machines, added debug() logging to make the polars variant decision explicit for future debugging, and moved the test_depends.py suite into the correct packages/ directory so it actually runs. I've also implemented the scoped sys.modules stub pattern for engLib to completely prevent pytest session pollution, cleaned up the formatting violations, and threw in an lru_cache on the AVX2 check to prevent redundant OS sysctl checks on startup.
Please have another review when you have a chance to.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/server/engine-lib/rocketlib-python/lib/depends.py`:
- Around line 808-811: Update the debug message in the non-exclusion else branch
of _is_x86_64_missing_avx2() so it does not claim AVX2 was detected on non-x86
architectures. Use wording that accurately describes selecting standard Polars
for all supported architectures while keeping the excludes update unchanged.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8b57b02a-93e2-4151-82e7-a0e915802677

📥 Commits

Reviewing files that changed from the base of the PR and between c6bff6c and 7dbe7c9.

📒 Files selected for processing (2)
  • packages/server/engine-lib/rocketlib-python/lib/depends.py
  • packages/server/engine-lib/rocketlib-python/tests/test_depends.py

Comment on lines +808 to +811
else:
debug('AVX2 instructions detected; using standard polars')
# Exclude the cpu variant so it isn't installed on standard AVX2 hardware
excludes += 'polars-lts-cpu\n'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Refine the debug message for non-x86 hardware.

When running on native non-x86 hardware like Apple Silicon (arm64), _is_x86_64_missing_avx2() correctly returns False to ensure standard polars is installed. However, the current debug log will inaccurately claim that "AVX2 instructions detected," even though ARM CPUs do not have AVX2 (they use NEON).

Consider tweaking the log message so it remains accurate for all architectures utilizing standard Polars.

💡 Proposed tweak
     else:
-        debug('AVX2 instructions detected; using standard polars')
+        debug('AVX2 fallback not required; using standard polars')
         # Exclude the cpu variant so it isn't installed on standard AVX2 hardware
         excludes += 'polars-lts-cpu\n'
📝 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.

Suggested change
else:
debug('AVX2 instructions detected; using standard polars')
# Exclude the cpu variant so it isn't installed on standard AVX2 hardware
excludes += 'polars-lts-cpu\n'
else:
debug('AVX2 fallback not required; using standard polars')
# Exclude the cpu variant so it isn't installed on standard AVX2 hardware
excludes += 'polars-lts-cpu\n'
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/server/engine-lib/rocketlib-python/lib/depends.py` around lines 808
- 811, Update the debug message in the non-exclusion else branch of
_is_x86_64_missing_avx2() so it does not claim AVX2 was detected on non-x86
architectures. Use wording that accurately describes selecting standard Polars
for all supported architectures while keeping the excludes update unchanged.

@kiet08hogit
kiet08hogit requested a review from joshuadarron July 17, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:nodes Python pipeline nodes module:server C++ engine and server components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OCR node fails: type object 'builtins.PySeries' has no attribute 'new_opt_f16'

4 participants