allenai / allenai/asta-autodiscovery
Always interpret plots outside the code-execution environment
- Ngôn ngữ chính
- Python
- Star
- 10
- Fork
- 2
- Merge trung bình
- 1 ngày 21 giờ
- Pull request đã merge (30 ngày)
- 11
Mô tả
Raised by @radamson while reviewing #68. Related: #55 (unify code execution on the asta-sandbox backend registry), which this should probably ride along with rather than precede.
## Problem
Plot interpretation happens in two completely different places depending on `--backend`, and one of them runs the vision model **inside the code-execution environment**.
### Current state
**`--backend process` / `modal` — analysis outside the sandbox (good)**
The sandbox returns figures as structured rich outputs. `ModalSandboxExecutor.execute_code_blocks` picks base64 PNGs out of `result.rich_outputs` and calls the vision model from the parent process (`_analyze_image`). Figures are then persisted by `run._write_rich_outputs` to `rich_outputs/ro__.json` and rendered into the HTML report.
**`--backend local` — analysis inside the executed code (bad)**
`CodeBlockWrapperTransform` prepends a generated `build_image_analysis_patch(...)` block to every code block the agent runs. That patch monkeypatches `plt.show` so that, inside the executed program:
- it re-encodes each open figure to base64,
- calls the vision model **from within the execution environment**,
- prints the analysis to stdout,
- and emits a token-usage record as a magic stdout line (`__AUTODISCOVERY_LLM_USAGE__` + JSON), which the parent later scrapes back out via `llm_usage.extract_local_image_usage_markers`.
Consequences specific to this path:
| | |
|---|---|
| Credentials | model credentials must be reachable *from inside* the execution environment |
| Trigger | only fires on `plt.show()`; a figure that is saved or left open is never analysed |
| Transport | analysis and usage travel as stdout text, with usage parsed from a marker string |
| Persistence | figures are **not** stored — `_get_executor_rich_outputs` returns `[]` because `LocalCommandLineCodeExecutor` has no `get_last_rich_outputs`, so no `rich_outputs/` entries and nothing in the report |
| Injection | agent-authored code is rewritten before execution, so what runs is not what the agent wrote |
So `local` is both the least isolated backend *and* the one that needs credentials inside the sandbox — the inverse of what you'd want.
## Desired state
**One path: the execution environment returns figures as data; the parent interprets them.**
- Every backend surfaces figures through the same rich-output channel, so `_analyze_image` in the parent is the only place a vision model is called.
- `build_image_analysis_patch`, `CodeBlockWrapperTransform`, `LOCAL_IMAGE_USAGE_MARKER` and `extract_local_image_usage_markers` all disappear. No code injection, no stdout scraping, no credentials in the sandbox.
- Figures are persisted for every backend, so the report and `rich_outputs/` are backend-independent.
- Because analysis is decoupled from `plt.show`, it can be more comprehensive than today: analyse every figure produced, and re-analyse from stored plots without re-running the experiment. That last part is @radamson's point — the plots are already on disk for the sandbox backends, so interpretation no longer has to happen inline with execution.
## Why this is cheap now
#68 already removed the worst of the duplication: the injected patch used to carry its own copy of Vertex auth, token refresh and model-name normalisation, and now just calls `autodiscovery.llm`. So the patch is small, and deleting it is mostly a matter of `local` gaining rich outputs.
## Sequencing
This is really a corollary of #55. That issue notes `local` is autogen's `LocalCommandLineCodeExecutor` while `process` is the workspace's own `ProcessIPythonBackend` — and it is precisely the IPython-kernel-shaped backends that produce rich outputs. Once `local` routes through the asta-sandbox registry (`inprocess`), it gets rich outputs for free and the patch has no reason to exist.
**Worth asking as part of #55: should `--backend local` exist at all?** `process` is also local — a subprocess on the same machine — just isolated, and it is the default. `local` differs only by having no isolation, and it is already unsupported for Copilot (`get_agents` rejects that combination). If `local` were dropped, this issue closes by deletion rather than by implementation.
## Acceptance
- [ ] Vision model is never invoked from inside the execution environment, on any backend.
- [ ] `build_image_analysis_patch` / `CodeBlockWrapperTransform` deleted, or `local` removed.
- [ ] `LOCAL_IMAGE_USAGE_MARKER` and its stdout-scraping parser deleted; image-analysis usage is recorded through `UsageTracker` like every other call.
- [ ] Figures persist to `rich_outputs/` and appear in the report regardless of backend.
- [ ] Analysis no longer depends on the agent calling `plt.show()`.
Also worth folding in @radamson's broader note that "the way figures are handled by autodiscovery in general isn't great" — the above is the structural half of that; figure quality and selection is a separate conversation.
Hướng dẫn đóng góp
Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này
Đánh giá
Issue này chưa được đánh giá.