Skip to content

Commit 05d7dec

Browse files
committed
fix(skill): address CodeRabbit review — shell-safe examples, doc accuracy, F541
Objective: A public reviewer (CodeRabbit) flagged copy-paste examples that break in a real shell plus a few doc/lint issues in the skill's reference files. Approach: Fix the verified findings; decline two by repo convention. - integration-examples.md: the remote-server example used <ANGLE_BRACKET> placeholders (a shell parses "<" as input redirection, so the "copy-pasteable" command failed before ODL ran) -> quoted env var; the JSON reader now uses a context manager; and a note documents that max_chars bounds size BETWEEN elements (a single element larger than max_chars is not split). - options-matrix.md: qualify "a hybrid server is required" for --hybrid-fallback (the run then completes via local Java as fallback output, not hybrid); and note that formula/picture enrichment needs the hybrid SERVER's --enrich-* flags, not just the client's --hybrid-mode full (CodeRabbit's proposed client-flag fix was wrong -- those are server startup flags). - sync-skill-refs.py: drop f-prefixes on placeholder-free strings (Ruff F541). Declined: pinning actions to commit SHAs (repo convention is floating tags -- the sibling workflows use actions/checkout@v7 / setup-python@v6 unpinned, so pinning only these two would be inconsistent) and adding a concurrency group (CodeRabbit itself marked it low value for these light jobs). Evidence: ran the fixed code, did not just edit it. - Extracted the fixed chunk_with_citations from the reference and ran it on real ODL JSON: the context-manager path works and chunks carry {page,bbox}; a 5000-char element with max_chars=1000 becomes its own oversized chunk (matches the new note). - Shell-safety before/after: old form -> "bash: PRIVATE_BIND_ADDRESS: No such file or directory"; fixed form resolves to --host 10.0.0.5. - sync-skill-refs.py still runs (drift 31=31, exit 0); f-prefixes gone; py_compile OK. - skill-gate PASS (fences, evals schema, cross-refs, compile, drift).
1 parent 203602e commit 05d7dec

3 files changed

Lines changed: 17 additions & 9 deletions

File tree

skills/odl-pdf/references/integration-examples.md

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,8 @@ def iter_elements(node):
140140
yield from iter_elements(v)
141141

142142
def chunk_with_citations(json_path, max_chars=1000):
143-
doc = json.loads(open(json_path, encoding="utf-8").read())
143+
with open(json_path, encoding="utf-8") as f:
144+
doc = json.load(f)
144145
chunks, buf, buf_len = [], [], 0
145146
for el in iter_elements(doc):
146147
text = _text_of(el).strip()
@@ -160,6 +161,9 @@ def chunk_with_citations(json_path, max_chars=1000):
160161

161162
# Each chunk carries the (page, bbox) of every element it contains, so a
162163
# retrieved chunk can cite the exact page and region it came from.
164+
# max_chars bounds size BETWEEN elements: a single element longer than max_chars
165+
# becomes its own chunk that exceeds the bound (elements are not split, to keep
166+
# each citation's (page, bbox) intact).
163167
```
164168

165169
### LangChain
@@ -285,13 +289,15 @@ opendataloader-pdf input.pdf \
285289
For multi-machine deployments, run the server on a GPU host and point clients at it.
286290

287291
```bash
288-
# GPU host — bind an explicit private address; the server has NO built-in auth
289-
opendataloader-pdf-hybrid --host <PRIVATE_BIND_ADDRESS> --port 5002
292+
# GPU host — bind an explicit private address; the server has NO built-in auth.
293+
# Set this to the host's actual private address (quoted so it is not parsed as a shell redirect).
294+
HYBRID_HOST=10.0.0.5
295+
opendataloader-pdf-hybrid --host "$HYBRID_HOST" --port 5002
290296

291297
# Client
292298
opendataloader-pdf input.pdf \
293299
--hybrid docling-fast \
294-
--hybrid-url http://<gpu-host>:5002 \
300+
--hybrid-url "http://$HYBRID_HOST:5002" \
295301
--hybrid-timeout 30000
296302
```
297303

skills/odl-pdf/references/options-matrix.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -111,8 +111,10 @@ from the output, check that `--hybrid-mode full` is set.
111111

112112
**2. `--hybrid` requires a running server**
113113

114-
Setting `--hybrid docling-fast` (or any non-`off` value) without a reachable hybrid server will
115-
cause requests to fail. Quick start:
114+
Setting `--hybrid docling-fast` (or any non-`off` value) without a reachable hybrid server makes
115+
requests fail — **unless `--hybrid-fallback` is set, in which case the run completes via the local
116+
Java path and returns fallback output (no hybrid OCR/enrichment), which is easy to mistake for a
117+
successful hybrid extraction** (VERIFY the enrichments actually landed). Quick start:
116118

117119
```bash
118120
pip install "opendataloader-pdf[hybrid]"
@@ -226,7 +228,7 @@ opendataloader-pdf input.pdf \
226228
--hybrid-url http://127.0.0.1:5002
227229
```
228230

229-
Do **not** add `--hybrid-fallback` here: this example's goal is enrichment, and on a backend error fallback would yield enrichment-less output that still "succeeds." Add it only when partial local output beats failure — and then verify the enrichments actually landed.
231+
This is the **client** command; the enrichments only run if the hybrid **server** was started with the matching flags (`--enrich-formula` and/or `--enrich-picture-description`) — `--hybrid-mode full` alone routes pages to the backend but enables neither (Gotcha 2). Do **not** add `--hybrid-fallback` here: this example's goal is enrichment, and on a backend error fallback would yield enrichment-less output that still "succeeds." Add it only when partial local output beats failure — and then verify the enrichments actually landed.
230232

231233
---
232234

skills/odl-pdf/scripts/sync-skill-refs.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,12 +175,12 @@ def main(argv: list[str] | None = None) -> int:
175175

176176
# Report drift
177177
if new_options:
178-
print(f"\nNEW options (in options.json, not in skill):")
178+
print("\nNEW options (in options.json, not in skill):")
179179
for name in new_options:
180180
print(f" - {name}")
181181

182182
if removed_options:
183-
print(f"\nREMOVED options (in skill, not in options.json):")
183+
print("\nREMOVED options (in skill, not in options.json):")
184184
for name in removed_options:
185185
print(f" - {name}")
186186

0 commit comments

Comments
 (0)