allenai / allenai/asta-autodiscovery

Always interpret plots outside the code-execution environment

Đang mở
#74 2 bình luận 0 reaction 1 người được giao Được @gas2own nhận Xem trên GitHub
enhancement
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á.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.