fix(ocr): prevent native binary crash and handle AVX2 CPUs (RR-1325)#1376
fix(ocr): prevent native binary crash and handle AVX2 CPUs (RR-1325)#1376kiet08hogit wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe 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. ChangesOCR dependency and AVX2-aware install
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 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
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
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
📒 Files selected for processing (3)
nodes/src/nodes/ocr/requirements.txtnodes/src/nodes/requirements.txtpackages/server/engine-lib/rocketlib-python/lib/depends.py
| content = inp.read() | ||
| if 'img2table' in content or 'polars' in content: | ||
| needs_polars_override = _is_x86_64_missing_avx2() |
There was a problem hiding this comment.
🩺 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.
| 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
left a comment
There was a problem hiding this comment.
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 polarsfor 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') |
There was a problem hiding this comment.
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:
polarsis excluded (line 808) → not installed.polars-lts-cpulives only in the compile/constraints output → nothing in the install set requires it → not installed.img2tablethen has no Polars backend →import polarsinnodes/src/nodes/ocr/IGlobal.pyfails (or resolution fails because the requiredpolarsis 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 |
There was a problem hiding this comment.
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.
| 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() |
There was a problem hiding this comment.
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') | ||
|
|
There was a problem hiding this comment.
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.
|
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
762ccb5 to
e72d6dd
Compare
|
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. Simulating older non-AVX2 CPU: Simulating modern AVX2 CPU: |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
nodes/src/nodes/requirements.txt (1)
26-32: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBoth Polars variants are declared unconditionally — verify they can't both land in the install set.
polars>=1.0.0andpolars-lts-cpu>=1.0.0are 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 touv pip compileoutput, not directly touv pip install -r <file>.A prior reviewer on this same PR already flagged that
_combine_requirements/_compile_constraintsonly 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_constraintscode, not shown in this diff) thatexcludes.txtwritten by_write_excludes_file()is actually applied at real install time — e.g. viauv 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 affectinguv 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
📒 Files selected for processing (3)
nodes/src/nodes/ocr/requirements.txtnodes/src/nodes/requirements.txtpackages/server/engine-lib/rocketlib-python/lib/depends.py
dsapandora
left a comment
There was a problem hiding this comment.
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_avx2usessubprocess.check_outputon Darwin — please double checksubprocessis already imported at the top ofdepends.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_avx2would 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.
|
Thank you @asclearuc for reviewing, I just pushed a new commit based on your request:
Let me know if we're good to merge! |
joshuadarron
left a comment
There was a problem hiding this comment.
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__.py → depends()), 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:
- The new tests never run.
test/engine-lib/rocketlib-python/test_depends.pysits in the repo-roottest/directory, and nothing executes pytest there — I traced everyrunPytestcall site in the build system, and each targets a package-local dir (nodes/test/,packages/ai/tests/,packages/client-*/tests/, ...). The engine-lib runner ispackages/server/scripts/tasks.js:1062, which targetspackages/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 testbox 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 --fixclears 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 | |||
There was a problem hiding this comment.
🔴 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() | ||
|
|
There was a problem hiding this comment.
🟡 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) |
There was a problem hiding this comment.
🟡 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(): |
There was a problem hiding this comment.
🔵 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).
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
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. |
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
packages/server/engine-lib/rocketlib-python/lib/depends.pypackages/server/engine-lib/rocketlib-python/tests/test_depends.py
| 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' |
There was a problem hiding this comment.
📐 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.
| 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.
Summary
img2tablefrom the OCR node's requirements to the base engine requirements (and removed redundantnumpydependencies) to prevent fatal native binary hot-swapping crashes mid-flight.depends.pyto detect older x86/amd64 processors and Apple Rosetta environments.polars-lts-cpuvariant and block standardpolarsfrom being installed on systems without AVX2 support, preventing the "Illegal instruction" startup crash.Type
fix
Testing
./builder testpassesChecklist
Linked Issue
Fixes #1325
Summary by CodeRabbit
New Features
img2table.Bug Fixes
Tests