UN-3575 [FEAT] Lookup enrichment support for agentic-table prompts (OSS side)#2189
UN-3575 [FEAT] Lookup enrichment support for agentic-table prompts (OSS side)#2189pk-zipstack wants to merge 6 commits into
Conversation
Pass the per-prompt lookup_config into the agentic_table executor's params so post-extraction lookup enrichment runs for agentic-table prompts on the deployment path. Sourced from the output dict (attached upstream at export); None when no lookup is assigned, so it's a no-op otherwise. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Length-preserving bounded-concurrency map used by per-row lookup enrichment (and reusable by other LLM call sites). max_workers=1 == sequential. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughAdds a bounded-concurrency, order-preserving ChangesParallel processing and agentic lookup
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
| Filename | Overview |
|---|---|
| workers/shared/parallel_map.py | Adds a shared helper for ordered, length-preserving bounded parallel mapping. |
| workers/file_processing/structure_tool_task.py | Passes prompt-level lookup configuration into agentic-table execution. |
| workers/executor/executors/lookup_enrichment.py | Adds optional lookup reference preloading and forwards preloaded references to the enrichment plugin. |
Reviews (5): Last reviewed commit: "UN-3575 [FIX] Guard preload_reference_te..." | Re-trigger Greptile
…okup # Conflicts: # workers/file_processing/structure_tool_task.py
| with backoff and honors ``Retry-After``, so ``max_workers`` concurrent callers | ||
| self-throttle without any extra coordination in this utility. | ||
|
|
||
| NOTE: there is no early-abort. All submitted work runs to completion even if the |
There was a problem hiding this comment.
@muhammad-ali-e do you have any thoughts on this behaviour?
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review — UN-3575 OSS side
Reviewed together with the cloud companion Zipstack/unstract-cloud#1662.
Both changes are appropriately small and the structure_tool_task.py forwarding is exactly right. parallel_map's core design — length-preserving, index-realigned, max_workers=1 collapsing to sequential so one knob covers both strategies — is the correct shape for the per-row use case, and the no-early-abort NOTE is a genuinely good comment.
Main items inline. The one that matters most:
parallel_map.py:12-15 — the docstring's justification for omitting rate limiting doesn't hold up against the SDK. 529 (Anthropic overloaded_error) isn't in RETRYABLE_STATUS_CODES, retries are config-gated rather than unconditional, and "concurrent callers self-throttle" doesn't follow from retry-with-backoff. Since that paragraph is the entire stated reason this utility ships without rate limiting, it's worth either correcting or just scoping the claim down.
Also worth a thought, though not blocking: parallel_map is a near-verbatim copy of the pre-existing workers/plugins/agentic_table/src/utils/parallel_map.py in the cloud repo (same ThreadPoolExecutor + as_completed + index-map structure), and today the only consumer is that same cloud plugin. Generalizing the plugin's own utility would avoid both the duplication and the new cross-repo import dependency. The OSS placement is defensible if other call sites are planned — just flagging the trade-off.
Tests
Not asking for full coverage, but parallel_map is pure, dependency-free and trivially testable, and it lands in a CI-gated path (unit-workers, coverage_source: shared). Four asserts would pin the whole contract: order preserved under reversed completion, length preserved on failure with on_error=None, on_error fallback lands at the failed index, and max_workers <= 1. The middle one is the single behaviour that differs from parallel_page_map (which ends with [r for r in results if r is not None]) and therefore the module's entire reason to exist. workers/plugins/agentic_table/tests/test_parallel_map.py in the cloud repo already covers all of these for the predecessor and ports almost directly.
Comments
Could you keep the new comments concise and generic? A few here narrate the PR's own context or reference symbols in the other repo, which won't read correctly to an OSS-only reader and will go stale as the code moves. Flagged inline.
parallel_map: return type is now list[R | None] (matches the None-on-omitted- on_error contract); drop the needless log lock (loop body is single-threaded); cap per-item tracebacks and add a failure summary so a systemic failure can't flood logs; guard on_error so a raising fallback can't hang the pool. Docstring corrected (drop cloud-only symbol reference; don't overstate SDK retry coverage). structure_tool_task: collapse the lookup_config comment and name the source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d hoist) Add preload_reference_texts() and a reference_texts passthrough on run_lookup_enrichment so per-row enrichment can read the reference file(s) once instead of once per row. None => each call loads its own (unchanged path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 `@workers/executor/executors/lookup_enrichment.py`:
- Around line 35-50: The preload_reference_texts function must return None when
the plugin preload operation fails instead of propagating exceptions. In
preload_reference_texts, verify the retrieved preload hook is callable before
invoking it, and catch preload/read failures to return None so
run_lookup_enrichment can fall back to per-call loading.
🪄 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: CHILL
Plan: Pro Plus
Run ID: ee0c4dd5-9331-4b5f-8634-7697d83ccf47
📒 Files selected for processing (1)
workers/executor/executors/lookup_enrichment.py
Honour the documented None-on-failure contract: wrap the plugin's preload call in try/except (and use callable()) so a raising plugin hook falls back to per-call reference loading instead of aborting enrichment. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Unstract test resultsPer-group results
Critical paths
|
What
lookup_configinto the agentic-table executor'sagentic_paramsinstructure_tool_task, so post-extraction lookup enrichment runs for agentic-table prompts on the deployment path.workers/shared/parallel_map.py, used by the agentic-table lookup per-row enrichment (and reusable by other LLM call sites).Why
Ref UN-3575. The cloud repo enables lookups on agentic-table prompts; the enrichment runs inside the dedicated
agentic_tableexecutor. For that to fire on deployed pipelines (not just the IDE), the OSSstructure_tool_task.py— which dispatches agentic-table prompts to the executor — must forward thelookup_configthat is attached to the prompt output upstream at export.The per-row enrichment mode needs bounded concurrency (N rows → N LLM calls, capped at K workers).
parallel_mapis generic OSS-side infrastructure (like the existingrun_lookup_enrichmenthook), so it lives in the OSSworkers/sharedand is imported from there by the cloud agentic executor at runtime.How
workers/file_processing/structure_tool_task.py: after building the baseagentic_params, forwardat_output.get("lookup_config"). It isNonewhen no lookup is assigned to the prompt, so the executor runs enrichment only when set.workers/shared/parallel_map.py(new):parallel_map(items, worker, *, max_workers, on_error=None, label="")— aThreadPoolExecutormap that returns results in input order and always the same length as the input (failed items hold theon_errorfallback, never dropped), so callers realign results to inputs by index.max_workers=1runs effectively sequentially, so one knob covers both sequential and bounded-parallel strategies. Retries/rate-limits are intentionally left to the SDK LLM client (which already backs off on 429/5xx and honoursRetry-After), so concurrent callers self-throttle. No early-abort (documented).Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
No. Both changes are additive:
at_output.get("lookup_config")returnsNonefor all current deployments (no lookup on agentic-table), so the executor's enrichment path is skipped andagentic_paramsis functionally unchanged.workers/shared/parallel_map.pyis a brand-new module with no existing importers in this repo; it introduces no behavioural change on its own.Database Migrations
per_rowfield live in the cloud repo).Env Config
LOOKUP_PER_ROW_MAX_WORKERS(default8) is read by the cloud agentic-table worker for per-row concurrency; no OSS-side config change required.Relevant Docs
Related Issues or PRs
lookup_configparam forwarded here and importsparallel_mapfrom this repo'sworkers/shared.Dependencies Versions
Notes on Testing
parallel_map: N items across up to 8 workersandPer-row lookup enrichment complete: N/N rows enriched, 0 failed.lookup_config=Noneand behave exactly as before.Screenshots
N/A — backend/worker-only change.
Checklist
I have read and understood the Contribution Guidelines.