Skip to content

Checking appworld specific changes compared to agents base#1753

Open
shatu wants to merge 13 commits into
omni_agentfrom
appworld-env
Open

Checking appworld specific changes compared to agents base#1753
shatu wants to merge 13 commits into
omni_agentfrom
appworld-env

Conversation

@shatu

@shatu shatu commented Jul 13, 2026

Copy link
Copy Markdown

No description provided.

shatu and others added 13 commits June 27, 2026 07:14
AppWorldEnv (config_name "appworld") trains a policy on AppWorld tasks via a
single execute_python tool. AppWorld pins pydantic<2 (open-instruct needs >=2),
so it runs as a per-rollout container (ghcr.io/stonybrooknlp/appworld:latest,
`appworld serve environment`) and the env is a thin HTTP client that never imports
appworld — mirroring the swerl podman workflow. Reward is AppWorld's own /evaluate
(fraction of unit tests passed). Includes a dataset converter, launch script,
smoke test, README, and unit tests. Validated end-to-end against a real container.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… beaker

EnvironmentPool only rotates SWERL_PODMAN_DOCKER_HOSTS when the env config has
backend == "docker". Add it to AppWorldEnvConfig so AppWorld containers land on
the podman hosts on beaker (no-op locally when the env var is unset).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…local 2-GPU debug run

- grpo_fast._discover_tools_from_datasets: handle local .jsonl/.parquet paths like
  DatasetConfig does (was FileNotFoundError on a local parquet).
- convert_appworld_to_rl: tools column must be the tool *function* name
  (["execute_python"]) so the schema is injected; env_config env_name must equal the
  pool/call name (execute_python), not the registry name (appworld) — the mismatch
  auto-created a duplicate pool and stalled the rollout handoff.
- Add scripts/general_agent/appworld/rl/local_rl_2gpu.sh: HF/Beaker-free local GRPO
  smoke run (Qwen3-0.6B, data-baked image, local parquet). Validated end to end.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…hatu/appworld-rl

convert_appworld_to_rl.py now takes --splits (comma list) and builds a DatasetDict
(one named split each) with an optional --private push. Launch script defaults to the
shatu/appworld-rl dataset (train for training, dev for eval).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pt (podman recipe)

- AppWorldEnv now always publishes the server port and probes both the published host
  port and the container bridge IP, picking whichever answers — robust across local
  docker, remote/podman, and Beaker sibling-container networking.
- Rewrite qwen35_4b_appworld.sh on the swerl podman-services recipe (BEAKER_ALLOW_SUBCONTAINERS,
  SWERL_PODMAN_SERVICE_COUNT, MIRROR_URL, docker_login + ray_node_setup), using the
  data-baked image shatu/appworld-data:latest and shatu/appworld-rl.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… reference)

Captures the env/tool/pool/container architecture, the RLVR dataset contract, the
Beaker podman recipe, the two integration styles (bash-exec vs stateful-server), the
AppWorld worked example, every integration trap, and a copy-paste checklist. Part 1 is
a plain-language mental model; Part 2 is the engineering reference.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…an container probe

Root-causing the 4h-idle Beaker run: every AppWorld container exited within ~1s and
auto_remove deleted the evidence, so each rollout reset failed -> zero-reward no-op ->
GPUs idle. swerl runs fine on the same podman shards but never publishes a host port
(it uses docker exec), so port-publishing under rootless/sharded podman is the prime suspect.

- AppWorldEnv: auto_remove=False + capture exit code/logs on early exit; publish_port=False
  by default (reach the server via the container bridge IP, swerl-consistent); memswap_limit
  like swerl; explicit container cleanup on failure/teardown.
- Add scripts/general_agent/appworld/debug/ probe: a no-training 1-node Beaker job that starts
  the container WITHOUT and WITH port publish and reports status/exit/logs/bridge-IP reachability.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…kSettings dump

Rootless podman gives the container no bridge IP and no publishable host port (server is
up but unreachable). Test whether network_mode=host makes it reachable at 127.0.0.1:<port>
from the trainer netns; dump NetworkSettings to confirm why IP is empty.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…an host networking)

Probe revealed Beaker's podman runs containers on the *host* network: no bridge IP, no
publishable host port, but the server IS reachable at 127.0.0.1:<port> from the trainer netns.
The old env only tried the (empty) bridge IP / published port, so every reset failed -> the
4h idle run. Fix: allocate a unique free port per container (host netns is shared, so ports
must not collide) and probe 127.0.0.1:<port> first, then the bridge IP (local docker fallback).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… network)

Beaker podman runs containers on the host network: no bridge IP, publish is a no-op, but the
server is reachable at 127.0.0.1:<unique-port> from the trainer netns. Update the networking
section + trap list with the verified finding and the probe/auto_remove diagnosis tips.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both integration styles are stateful; the deciding axis is where the state lives and whether a
fresh process can see it — filesystem (+ serialized cwd/env) -> docker-exec (A); live in-memory
session -> long-lived server + client (B). Add the swerl cwd/env-serialization detail and a note
that style B scales: container+port are created once per pool actor and reused, not per rollout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… inflight_updates off

First run wedged after 1 step (weight-sync hang) with acquire_reset_pools 150-260s/rollout
(pool contention + heavy AppWorld init) and 24k-token truncated rollouts. Shakeout: 8x8=64
concurrent, pool_size 96, max_steps/max_interactions 20, response_length 16384, per_turn 4096,
drop active_sampling (bound concurrency), inflight_updates false (sync between steps).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n-fatal

The host-net design picks a free port then the container binds it; with many concurrent
containers another can grab the port in between -> "address already in use" -> server exits ->
reset crashed the run. Retry _start_container with a fresh port each attempt (start_retries=5,
removing the dead container between tries); on exhaustion raise with "reset failed after" so the
rollout is zero-rewarded (RESET_FAILURE_ZERO_REWARD) instead of killing the job.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new containerized, stateful RL environment for the AppWorld benchmark, along with comprehensive documentation, dataset conversion scripts, diagnostic tools, and unit tests. The reviewer's feedback highlights a critical performance and stability issue: the asynchronous methods of AppWorldEnv (reset, step, and close) perform synchronous, blocking Docker and HTTP operations directly on the main event loop. To prevent blocking the Ray actor's event loop and causing potential heartbeat timeouts, the reviewer recommends wrapping these blocking calls in asyncio.to_thread and adding robust error handling in step to gracefully handle container or network failures.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +31 to +45
import contextlib
import socket
import time
import uuid
from dataclasses import dataclass
from typing import Any, ClassVar
from urllib.parse import urlparse

import requests
from openenv.core.env_server.types import State

from open_instruct import logger_utils

from .base import BaseEnvConfig, EnvCall, RLEnvironment, StepResult
from .tools.utils import coerce_args

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Import asyncio to allow running synchronous, blocking Docker and HTTP requests in a separate thread using asyncio.to_thread. This prevents blocking the main event loop of the Ray actor.

Suggested change
import contextlib
import socket
import time
import uuid
from dataclasses import dataclass
from typing import Any, ClassVar
from urllib.parse import urlparse
import requests
from openenv.core.env_server.types import State
from open_instruct import logger_utils
from .base import BaseEnvConfig, EnvCall, RLEnvironment, StepResult
from .tools.utils import coerce_args
import asyncio
import contextlib
import socket
import time
import uuid
from dataclasses import dataclass
from typing import Any, ClassVar
from urllib.parse import urlparse
import requests
from openenv.core.env_server.types import State
from open_instruct import logger_utils
from .base import BaseEnvConfig, EnvCall, RLEnvironment, StepResult
from .tools.utils import coerce_args

Comment on lines +398 to +434
async def reset(self, task_id: str | None = None, **kwargs: Any) -> tuple[StepResult, list[dict]]:
if self._client is None:
await self.setup()
if task_id is None:
raise ValueError("AppWorldEnv.reset requires a task_id (set it via env_config).")
# Allow the pool to rotate docker hosts on retry (mirrors swerl).
if kwargs.get("docker_host") and kwargs["docker_host"] != self._docker_host:
self._docker_host = kwargs["docker_host"]
self._teardown_container()
await self.setup()
if self._container is None or self._base_url is None:
self._start_container()

self._step_count = 0
self._completed = False
self._last_reward = 0.0
self._last_eval = {}
self._task_id = task_id
self._max_steps = kwargs.get("max_steps", self._max_interactions)
episode = kwargs.get("episode_id") or kwargs.get("instance_id") or uuid.uuid4().hex
self._experiment_name = f"{self._experiment_prefix}_{task_id}_{episode}"

# Re-initialize the container's single world to this task.
self._http(
"initialize",
task_id=task_id,
experiment_name=self._experiment_name,
max_interactions=self._max_interactions,
timeout_seconds=self._timeout_seconds,
load_ground_truth=True, # required for /evaluate
raise_on_unsafe_syntax=True,
null_patch_unsafe_execution=True,
raise_on_failure=False,
)
# The reset observation is discarded by the rollout pipeline; the prompt
# comes from the dataset messages (see build_prompt_messages).
return StepResult(result="", metadata={"task_id": task_id}), list(self._tool_definitions)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The reset method is asynchronous, but it calls synchronous, blocking operations like _teardown_container, _start_container, and _http directly on the event loop. This can block the event loop for up to 180 seconds (the startup timeout), which will likely cause Ray actor heartbeat timeouts and performance degradation. Wrap these blocking calls in asyncio.to_thread to run them in a separate thread.

    async def reset(self, task_id: str | None = None, **kwargs: Any) -> tuple[StepResult, list[dict]]:
        if self._client is None:
            await self.setup()
        if task_id is None:
            raise ValueError("AppWorldEnv.reset requires a task_id (set it via env_config).")
        # Allow the pool to rotate docker hosts on retry (mirrors swerl).
        if kwargs.get("docker_host") and kwargs["docker_host"] != self._docker_host:
            self._docker_host = kwargs["docker_host"]
            await asyncio.to_thread(self._teardown_container)
            await self.setup()
        if self._container is None or self._base_url is None:
            await asyncio.to_thread(self._start_container)

        self._step_count = 0
        self._completed = False
        self._last_reward = 0.0
        self._last_eval = {}
        self._task_id = task_id
        self._max_steps = kwargs.get("max_steps", self._max_interactions)
        episode = kwargs.get("episode_id") or kwargs.get("instance_id") or uuid.uuid4().hex
        self._experiment_name = f"{self._experiment_prefix}_{task_id}_{episode}"

        # Re-initialize the container's single world to this task.
        await asyncio.to_thread(
            self._http,
            "initialize",
            task_id=task_id,
            experiment_name=self._experiment_name,
            max_interactions=self._max_interactions,
            timeout_seconds=self._timeout_seconds,
            load_ground_truth=True,  # required for /evaluate
            raise_on_unsafe_syntax=True,
            null_patch_unsafe_execution=True,
            raise_on_failure=False,
        )
        # The reset observation is discarded by the rollout pipeline; the prompt
        # comes from the dataset messages (see build_prompt_messages).
        return StepResult(result="", metadata={"task_id": task_id}), list(self._tool_definitions)

Comment on lines +436 to +469
async def step(self, call: EnvCall) -> StepResult:
if self._base_url is None or self._task_id is None:
raise RuntimeError("Environment not reset. Call reset() first.")
self._step_count += 1

if call.name != "execute_python":
return StepResult(
result=f"Unknown tool '{call.name}'. The only available tool is `execute_python`."
)

args = coerce_args(EXECUTE_TOOL["function"]["parameters"], call.args)
code = args.get("code", "")
if not code or not code.strip():
return StepResult(result="Error: the `code` parameter is required and must be non-empty.")

output = self._http("execute", task_id=self._task_id, code=code)
observation = truncate_observation(output) if output else "(no output)"

self._completed = bool(self._http("task_completed", task_id=self._task_id))
at_step_limit = self._max_steps is not None and self._step_count >= self._max_steps
if self._completed or at_step_limit:
reward = self._compute_reward()
summary = (
f"\n\n[Task {'completed' if self._completed else 'truncated at step limit'}. "
f"Reward {reward:.3f} "
f"({self._last_eval.get('pass_count', 0):.0f}/{self._last_eval.get('num_tests', 0):.0f} tests passed).]"
)
return StepResult(
result=observation + summary,
reward=reward,
done=True,
metadata={"task_id": self._task_id, "completed": self._completed, **self._last_eval},
)
return StepResult(result=observation, metadata={"task_id": self._task_id, "completed": False})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The step method is asynchronous, but it calls synchronous, blocking HTTP requests directly on the event loop. Wrap these calls in asyncio.to_thread to prevent blocking the event loop. Additionally, wrap the HTTP calls in a try...except block to gracefully handle container crashes, OOMs, or network timeouts by returning an error observation and ending the episode with 0 reward, rather than crashing the entire training process.

    async def step(self, call: EnvCall) -> StepResult:
        if self._base_url is None or self._task_id is None:
            raise RuntimeError("Environment not reset. Call reset() first.")
        self._step_count += 1

        if call.name != "execute_python":
            return StepResult(
                result=f"Unknown tool '{call.name}'. The only available tool is `execute_python`."
            )

        args = coerce_args(EXECUTE_TOOL["function"]["parameters"], call.args)
        code = args.get("code", "")
        if not code or not code.strip():
            return StepResult(result="Error: the `code` parameter is required and must be non-empty.")

        try:
            output = await asyncio.to_thread(self._http, "execute", task_id=self._task_id, code=code)
            observation = truncate_observation(output) if output else "(no output)"
            self._completed = bool(await asyncio.to_thread(self._http, "task_completed", task_id=self._task_id))
        except Exception as e:
            logger.error(f"[{self._task_id}] AppWorld HTTP call failed: {e}")
            return StepResult(
                result=f"Error: Failed to communicate with the AppWorld environment server: {e}",
                reward=0.0,
                done=True,
                metadata={"task_id": self._task_id, "error": str(e)},
            )

        at_step_limit = self._max_steps is not None and self._step_count >= self._max_steps
        if self._completed or at_step_limit:
            reward = await asyncio.to_thread(self._compute_reward)
            summary = (
                f"\n\n[Task {'completed' if self._completed else 'truncated at step limit'}. "
                f"Reward {reward:.3f} "
                f"({self._last_eval.get('pass_count', 0):.0f}/{self._last_eval.get('num_tests', 0):.0f} tests passed).]"
            )
            return StepResult(
                result=observation + summary,
                reward=reward,
                done=True,
                metadata={"task_id": self._task_id, "completed": self._completed, **self._last_eval},
            )
        return StepResult(result=observation, metadata={"task_id": self._task_id, "completed": False})

Comment on lines +505 to +506
async def close(self) -> None:
self._teardown_container()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The close method is asynchronous, but it calls synchronous, blocking operations like _teardown_container directly on the event loop. Wrap this call in asyncio.to_thread to prevent blocking the event loop.

Suggested change
async def close(self) -> None:
self._teardown_container()
async def close(self) -> None:
await asyncio.to_thread(self._teardown_container)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant