Add Env EB-Alfred - #90
Open
YaningDylan wants to merge 29 commits into
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR adds complete EB-ALFRED environment integration to VAGEN, including a GymImageEnv adapter, multi-GPU load-balancing server with capacity-based queuing, prompting utilities, and evaluation/training configurations for embodied AI benchmarking. Changes
Sequence DiagramsequenceDiagram
participant Client as Remote Client
participant API as FastAPI Server
participant Handler as EbAlfredHandler
participant Queue as Session Queue
participant BgTask as Background Task
participant EbAlf as EbAlfred Env
participant GPU as GPU/Display
Client->>API: POST /connect (env_config)
API->>Handler: connect()
alt Capacity Available
Handler->>EbAlf: create_env() on least-loaded GPU
EbAlf->>GPU: assign display, init EBAlfEnv
GPU-->>EbAlf: ready
EbAlf-->>Handler: env instance
else Capacity Exceeded
Handler->>Queue: enqueue session
Handler-->>API: queued session
BgTask->>Queue: poll readiness
Queue->>EbAlf: deferred create_env()
end
API-->>Client: session_id
Client->>API: POST /step (session_id, action)
API->>Handler: call(session_id, action)
Handler->>EbAlf: step(action_str)
EbAlf->>GPU: execute action
GPU-->>EbAlf: observation, reward, done
EbAlf-->>Handler: step result
Handler-->>API: response
API-->>Client: observation, reward, done, info
Client->>API: POST /close (session_id)
API->>Handler: aclose()
Handler->>BgTask: cleanup
BgTask->>EbAlf: close() with timeout
EbAlf->>GPU: release display
GPU-->>EbAlf: closed
Handler->>Queue: release capacity slot
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
vagen/envs/eb_alfred/utils/prompt.py (1)
56-58: Keep the max-action instruction dynamic instead of hardcoding20.Line 57 hardcodes “20 actions,” while the actual limit is configurable via
max_actions_per_step. If that setting changes, prompt instructions become inconsistent.♻️ Suggested change
-1. Output a plan of actions. Each plan should include no more than 20 actions. +1. Output a plan of actions. Each plan should include no more than the configured maximum number of actions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/utils/prompt.py` around lines 56 - 58, The Guidelines text currently hardcodes “20 actions”; change the prompt construction to inject the configured limit from max_actions_per_step instead of the literal “20” (e.g., use string formatting or f-string when building the Guidelines block in prompt.py), so any change to the max_actions_per_step setting is reflected in the prompt; locate where the Guidelines string is composed in prompt.py and replace the hardcoded “20 actions” with a dynamic reference to max_actions_per_step (ensuring correct grammar/number formatting).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vagen/envs/eb_alfred/eb_alfred_env.py`:
- Around line 120-123: The system prompt always includes examples because
system_prompt(...) is called without respecting the use_example_in_sys_prompt
flag; update the calls that build the system prompt (e.g., the call that assigns
sys_str and the other system_prompt invocation around the same block) to pass
add_task_examples=use_example_in_sys_prompt so the value controls whether
examples are included (ensure use_example_in_sys_prompt is in scope and
forwarded to system_prompt just like format_prompt).
- Around line 82-85: The global X_DISPLAY assignment in ebalfenv_mod
(ebalfenv_mod.X_DISPLAY) must be protected with a lock to avoid races during
concurrent create_env()/EbAlfred initializations; introduce a module-level
threading.Lock (or reuse an existing lock) and acquire it around the mutation
and the subsequent EBAlfEnv-related initialization/constructor call (the region
that sets ebalfenv_mod.X_DISPLAY and then imports/instantiates
EBAlfEnv/EbAlfred) so only one thread can set the display and construct the
environment at a time; ensure the lock is released after the assignment +
constructor to avoid blocking other operations.
In `@vagen/envs/eb_alfred/handler.py`:
- Around line 69-77: The current _least_loaded_display method reads load only
from self._sessions which is race-prone during concurrent create_env() calls;
add an async-protected in-flight reservation tracking structure (e.g.,
self._display_reservations: Dict[display, int] and an asyncio.Lock or
asyncio.Semaphore used in create_env() flow) and modify _least_loaded_display to
include reservations when building counts (sum sessions from self._sessions +
reservations from self._display_reservations). Ensure create_env() acquires the
lock to increment the reservation for the chosen display before completing
environment creation and decrements it on success/failure so concurrent creators
see accurate in-flight counts; update any other code paths that pick a display
(the same logic also applies to the block referenced at lines 86-94) to use the
new combined counting logic.
- Around line 66-67: Trim and normalize the incoming x_displays before assigning
to self._x_displays: if x_displays is a string convert it to a single-item list,
if it's an iterable strip whitespace from each entry and remove empty/None
values, then if the resulting list is empty call detect_gpu_displays() and
assign its result; if that result is still empty, log an error via LOGGER.error
and raise a ValueError to fail fast. Update the assignment site that currently
uses self._x_displays = x_displays if x_displays is not None else
detect_gpu_displays() and the subsequent LOGGER.info call to use the
sanitized/validated list. Ensure behavior is consistent for both provided and
default display discovery.
In `@vagen/envs/eb_alfred/serve.py`:
- Line 36: Change the parser argument so the server binds to loopback by
default: update parser.add_argument("--host", ...) to default to "127.0.0.1" and
revise the help text accordingly; additionally add an explicit opt-in flag
(e.g., parser.add_argument("--allow-remote", action="store_true")) and, where
the host value is used to start the server, override host to "0.0.0.0" only when
allow_remote is true (or validate that a non-loopback host was intentionally
provided), so remote exposure requires explicit approval.
In `@vagen/envs/eb_alfred/utils/utils.py`:
- Around line 152-154: The current check only verifies numpy_array.shape[-1] ==
3 which allows non-HWC arrays to slip through; before calling Image.fromarray
validate that numpy_array is a 3-dimensional HxWxC array (e.g., numpy_array.ndim
== 3 and numpy_array.shape[2] == 3) and raise a clear ValueError if ndim or
channel position is wrong, and keep the existing branch that converts to uint8
and calls Image.fromarray; reference the numpy_array variable and
Image.fromarray in your update so the check is inserted immediately before the
conversion logic.
---
Nitpick comments:
In `@vagen/envs/eb_alfred/utils/prompt.py`:
- Around line 56-58: The Guidelines text currently hardcodes “20 actions”;
change the prompt construction to inject the configured limit from
max_actions_per_step instead of the literal “20” (e.g., use string formatting or
f-string when building the Guidelines block in prompt.py), so any change to the
max_actions_per_step setting is reflected in the prompt; locate where the
Guidelines string is composed in prompt.py and replace the hardcoded “20
actions” with a dynamic reference to max_actions_per_step (ensuring correct
grammar/number formatting).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/evaluate/eb_alfred/config.yamlvagen/envs/eb_alfred/__init__.pyvagen/envs/eb_alfred/eb_alfred_env.pyvagen/envs/eb_alfred/handler.pyvagen/envs/eb_alfred/serve.pyvagen/envs/eb_alfred/utils/__init__.pyvagen/envs/eb_alfred/utils/prompt.pyvagen/envs/eb_alfred/utils/utils.py
Comment on lines
+66
to
+67
| self._x_displays = x_displays if x_displays is not None else detect_gpu_displays() | ||
| LOGGER.info(f"[Handler] Using X displays: {self._x_displays}") |
Contributor
There was a problem hiding this comment.
Validate and normalize x_displays before use.
If x_displays is empty/contains blank entries, selection can become invalid ("") or fail at runtime. Sanitize and fail fast.
✅ Suggested change
- self._x_displays = x_displays if x_displays is not None else detect_gpu_displays()
+ if x_displays is None:
+ self._x_displays = detect_gpu_displays()
+ else:
+ cleaned = [d.strip() for d in x_displays if d and d.strip()]
+ if not cleaned:
+ raise ValueError("x_displays must contain at least one valid display id")
+ self._x_displays = cleaned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/handler.py` around lines 66 - 67, Trim and normalize the
incoming x_displays before assigning to self._x_displays: if x_displays is a
string convert it to a single-item list, if it's an iterable strip whitespace
from each entry and remove empty/None values, then if the resulting list is
empty call detect_gpu_displays() and assign its result; if that result is still
empty, log an error via LOGGER.error and raise a ValueError to fail fast. Update
the assignment site that currently uses self._x_displays = x_displays if
x_displays is not None else detect_gpu_displays() and the subsequent LOGGER.info
call to use the sanitized/validated list. Ensure behavior is consistent for both
provided and default display discovery.
| def main(): | ||
| parser = argparse.ArgumentParser(description="EB-ALFRED Remote Environment Server") | ||
| parser.add_argument("--port", type=int, default=8000, help="Server port") | ||
| parser.add_argument("--host", type=str, default="0.0.0.0", help="Server host") |
Contributor
There was a problem hiding this comment.
Avoid binding to all interfaces by default.
Line 36 defaults to 0.0.0.0, which exposes the service externally unless operators harden deployment. Prefer loopback by default and require explicit opt-in for remote exposure.
🔐 Suggested change
- parser.add_argument("--host", type=str, default="0.0.0.0", help="Server host")
+ parser.add_argument("--host", type=str, default="127.0.0.1", help="Server host")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| parser.add_argument("--host", type=str, default="0.0.0.0", help="Server host") | |
| parser.add_argument("--host", type=str, default="127.0.0.1", help="Server host") |
🧰 Tools
🪛 Ruff (0.15.2)
[error] 36-36: Possible binding to all interfaces
(S104)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/serve.py` at line 36, Change the parser argument so the
server binds to loopback by default: update parser.add_argument("--host", ...)
to default to "127.0.0.1" and revise the help text accordingly; additionally add
an explicit opt-in flag (e.g., parser.add_argument("--allow-remote",
action="store_true")) and, where the host value is used to start the server,
override host to "0.0.0.0" only when allow_remote is true (or validate that a
non-loopback host was intentionally provided), so remote exposure requires
explicit approval.
Comment on lines
+152
to
+154
| if numpy_array.shape[-1] == 3: | ||
| return Image.fromarray(numpy_array.astype(np.uint8), mode="RGB") | ||
| raise ValueError(f"Unsupported channels: {numpy_array.shape[-1]}. Expected 3 (RGB).") |
Contributor
There was a problem hiding this comment.
Validate image dimensionality before RGB conversion.
shape[-1] == 3 alone is too weak. Non-HWC arrays can pass this check and then fail in Image.fromarray with less actionable errors.
🛠️ Suggested change
def numpy_to_pil(numpy_array: np.ndarray) -> Image.Image:
"""Convert numpy (H, W, 3) to PIL.Image in RGB."""
- if numpy_array.shape[-1] == 3:
+ if numpy_array.ndim == 3 and numpy_array.shape[2] == 3:
return Image.fromarray(numpy_array.astype(np.uint8), mode="RGB")
raise ValueError(f"Unsupported channels: {numpy_array.shape[-1]}. Expected 3 (RGB).")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if numpy_array.shape[-1] == 3: | |
| return Image.fromarray(numpy_array.astype(np.uint8), mode="RGB") | |
| raise ValueError(f"Unsupported channels: {numpy_array.shape[-1]}. Expected 3 (RGB).") | |
| if numpy_array.ndim == 3 and numpy_array.shape[2] == 3: | |
| return Image.fromarray(numpy_array.astype(np.uint8), mode="RGB") | |
| raise ValueError(f"Unsupported channels: {numpy_array.shape[-1]}. Expected 3 (RGB).") |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 154-154: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/utils/utils.py` around lines 152 - 154, The current
check only verifies numpy_array.shape[-1] == 3 which allows non-HWC arrays to
slip through; before calling Image.fromarray validate that numpy_array is a
3-dimensional HxWxC array (e.g., numpy_array.ndim == 3 and numpy_array.shape[2]
== 3) and raise a clear ValueError if ndim or channel position is wrong, and
keep the existing branch that converts to uint8 and calls Image.fromarray;
reference the numpy_array variable and Image.fromarray in your update so the
check is inserted immediately before the conversion logic.
YaningDylan
force-pushed
the
add-env-eb-alfred
branch
from
March 9, 2026 08:24
f23bc02 to
d588c79
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
vagen/envs/eb_alfred/serve.py (1)
36-36:⚠️ Potential issue | 🟠 MajorDon't expose the env server on
0.0.0.0by default.This service is unauthenticated in the shown code, so binding all interfaces makes remote access the default. Prefer loopback by default and require explicit opt-in for external exposure.
🛠️ Minimal fix
- parser.add_argument("--host", type=str, default="0.0.0.0", help="Server host") + parser.add_argument("--host", type=str, default="127.0.0.1", help="Server host")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/serve.py` at line 36, The CLI parser currently defaults the --host argument to "0.0.0.0", exposing the unauthenticated env server to all interfaces; change the default to the loopback address "127.0.0.1" in the parser.add_argument call (refer to the --host argument in serve.py) and update the help text to mention that external exposure requires explicitly setting the host to 0.0.0.0 or another external interface.vagen/envs/eb_alfred/eb_alfred_env.py (1)
173-176:⚠️ Potential issue | 🟠 MajorForward
use_example_in_sys_promptintosystem_prompt().
format_prompt()respects the flag, butsystem_prompt()still uses its defaultadd_task_examples=True, so examples are always included in the system section.🛠️ Proposed fix
sys_str = system_prompt( task_instruction=self.env.episode_language_instruction, action_list=self._action_list, + add_task_examples=self.config.use_example_in_sys_prompt, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/eb_alfred_env.py` around lines 173 - 176, The system prompt is always including examples because system_prompt() is called without forwarding the use_example_in_sys_prompt flag; update the call that builds sys_str in eb_alfred_env.py to pass use_example_in_sys_prompt=self.use_example_in_sys_prompt (or the relevant flag name) into system_prompt(), ensuring system_prompt(...) receives and uses that parameter (matching format_prompt's behavior) so examples are conditionally included; verify system_prompt signature supports add_task_examples/use_example_in_sys_prompt or update the signature accordingly to propagate the flag from format_prompt to system_prompt.vagen/envs/eb_alfred/utils/utils.py (1)
150-154:⚠️ Potential issue | 🟡 MinorValidate HWC shape before calling
Image.fromarray.
shape[-1] == 3is still too weak here. Non-HWC arrays can pass this check and then fail deeper in PIL with a less actionable error.🛠️ Proposed fix
def numpy_to_pil(numpy_array: np.ndarray) -> Image.Image: """Convert numpy (H, W, 3) to PIL.Image in RGB.""" - if numpy_array.shape[-1] == 3: + if numpy_array.ndim == 3 and numpy_array.shape[2] == 3: return Image.fromarray(numpy_array.astype(np.uint8), mode="RGB") raise ValueError(f"Unsupported channels: {numpy_array.shape[-1]}. Expected 3 (RGB).")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/utils/utils.py` around lines 150 - 154, The numpy_to_pil function currently only checks shape[-1] == 3 which allows non-HWC arrays to slip through; update numpy_to_pil to explicitly validate the array is 3-dimensional with shape (H, W, 3) (e.g., check numpy_array.ndim == 3 and numpy_array.shape[2] == 3), raise a clear ValueError if that check fails, and then proceed to call Image.fromarray with numpy_array.astype(np.uint8) as before; reference the numpy_to_pil function to locate where to add the ndim/shape validation and the improved error message.vagen/envs/eb_alfred/handler.py (1)
105-107:⚠️ Potential issue | 🟡 MinorNormalize
x_displaysbefore building load state.Whitespace and blank entries still flow through here. For example,
--x-displays 0, 1leaves" 1"untouched, and an empty list can later make_least_loaded_display()fail onmin().🛠️ Proposed fix
- self._x_displays = x_displays if x_displays is not None else detect_gpu_displays() + if x_displays is None: + self._x_displays = detect_gpu_displays() + else: + self._x_displays = [d.strip() for d in x_displays if d and d.strip()] + if not self._x_displays: + raise ValueError("x_displays must contain at least one valid display id")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/handler.py` around lines 105 - 107, Normalize and validate x_displays before building load state: when initializing (the __init__ that sets self._x_displays and self._pending_counts) strip whitespace from each entry, split comma strings if needed, remove empty/blank entries (e.g., filter out "" and strings consisting only of whitespace), and if the resulting list is empty either fall back to detect_gpu_displays() or raise a clear ValueError; then construct self._pending_counts from the cleaned list so _least_loaded_display() and min() calls are safe. Ensure this normalization handles both provided x_displays and the detect_gpu_displays() result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/evaluate/eb_alfred/config.yaml`:
- Line 23: The config uses the wrong key name: EnvSpec expects concat_multi_turn
but the file sets concat_history, so the no-concat behavior is ignored; update
the config to replace concat_history: false with concat_multi_turn: false
(keeping the same semantics) so EnvSpec will pick up the intended no-concat
setting.
In `@vagen/envs/eb_alfred/handler.py`:
- Around line 174-177: The unlimited-capacity early-return in connect (the
branch where self._capacity_sem is None after calling self._ensure_semaphore and
it returns await super().connect(env_config, seed=seed)) yields a plain
SessionContext missing the _holds_slot attribute that _handle_close,
_cleanup_loop, and aclose expect; modify this path so the returned context is
compatible by either wrapping the super().connect() result into the same
LimitedSessionContext (or whatever subclass is used elsewhere) or set
ctx._holds_slot = False on the returned context before returning, ensuring the
symbol connect() returns an object with the _holds_slot attribute to avoid
AttributeError during _handle_close/_cleanup_loop/aclose.
- Around line 203-257: When spawning the background task, store it on the
session context (set ctx._create_task =
asyncio.create_task(self._deferred_create(ctx))) and ensure all close/cleanup
paths (_handle_close, _cleanup_loop, aclose) cancel and await ctx._create_task
before proceeding; inside _deferred_create, after each wait point (after
awaiting self._capacity_sem.acquire(), after acquiring self._startup_sem, and
after create_env returns) re-check that ctx.session_id is still present in
self._sessions and if it is not, immediately close the newly created env (if
any), release any held semaphores (self._startup_sem and/or self._capacity_sem)
and clear ctx._holds_slot; always set ctx._ready and ensure exceptions still set
ctx._error and release capacity slot if held to avoid leaking slots or Unity
processes.
In `@vagen/envs/eb_alfred/README.md`:
- Around line 111-125: The README examples reference stale config paths (e.g.,
tests/eval_eb_alfred_gpt41_20ep.yaml) but the PR adds configs under
examples/evaluate/eb_alfred/config.yaml; update the example usage and the
"Available Configs" table in vagen/envs/eb_alfred/README.md to point to the new
paths (replace any tests/...yaml entries with the corresponding
examples/evaluate/eb_alfred/... YAML filenames) and ensure the --config example
uses examples/evaluate/eb_alfred/config.yaml or the specific new filenames so
users are directed to the actual config files added in this PR.
In `@vagen/envs/eb_alfred/utils/prompt.py`:
- Around line 163-177: The wm_format_prompt() text contradicts multi-action
mode: update the prompt (inside wm_format_prompt) so when max_actions_per_step >
1 it instructs the model to output up to max_actions_per_step actions in the
<answer> tag separated by the provided action_sep; explicitly mention
action_sep, allow multiple action names or IDs (up to max_actions_per_step), and
remove the “exactly 1 action” wording; ensure the prompt aligns with parse_wm()
behavior by describing the separator and the allowed cardinality for <answer>.
---
Duplicate comments:
In `@vagen/envs/eb_alfred/eb_alfred_env.py`:
- Around line 173-176: The system prompt is always including examples because
system_prompt() is called without forwarding the use_example_in_sys_prompt flag;
update the call that builds sys_str in eb_alfred_env.py to pass
use_example_in_sys_prompt=self.use_example_in_sys_prompt (or the relevant flag
name) into system_prompt(), ensuring system_prompt(...) receives and uses that
parameter (matching format_prompt's behavior) so examples are conditionally
included; verify system_prompt signature supports
add_task_examples/use_example_in_sys_prompt or update the signature accordingly
to propagate the flag from format_prompt to system_prompt.
In `@vagen/envs/eb_alfred/handler.py`:
- Around line 105-107: Normalize and validate x_displays before building load
state: when initializing (the __init__ that sets self._x_displays and
self._pending_counts) strip whitespace from each entry, split comma strings if
needed, remove empty/blank entries (e.g., filter out "" and strings consisting
only of whitespace), and if the resulting list is empty either fall back to
detect_gpu_displays() or raise a clear ValueError; then construct
self._pending_counts from the cleaned list so _least_loaded_display() and min()
calls are safe. Ensure this normalization handles both provided x_displays and
the detect_gpu_displays() result.
In `@vagen/envs/eb_alfred/serve.py`:
- Line 36: The CLI parser currently defaults the --host argument to "0.0.0.0",
exposing the unauthenticated env server to all interfaces; change the default to
the loopback address "127.0.0.1" in the parser.add_argument call (refer to the
--host argument in serve.py) and update the help text to mention that external
exposure requires explicitly setting the host to 0.0.0.0 or another external
interface.
In `@vagen/envs/eb_alfred/utils/utils.py`:
- Around line 150-154: The numpy_to_pil function currently only checks shape[-1]
== 3 which allows non-HWC arrays to slip through; update numpy_to_pil to
explicitly validate the array is 3-dimensional with shape (H, W, 3) (e.g., check
numpy_array.ndim == 3 and numpy_array.shape[2] == 3), raise a clear ValueError
if that check fails, and then proceed to call Image.fromarray with
numpy_array.astype(np.uint8) as before; reference the numpy_to_pil function to
locate where to add the ndim/shape validation and the improved error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15a6ecfa-01a4-4c5d-a5c3-4966ba9e5a02
📒 Files selected for processing (10)
examples/evaluate/eb_alfred/config.yamlvagen/envs/eb_alfred/README.mdvagen/envs/eb_alfred/__init__.pyvagen/envs/eb_alfred/eb_alfred_env.pyvagen/envs/eb_alfred/handler.pyvagen/envs/eb_alfred/serve.pyvagen/envs/eb_alfred/utils/__init__.pyvagen/envs/eb_alfred/utils/prompt.pyvagen/envs/eb_alfred/utils/utils.pyvagen/envs_remote/handler.py
Comment on lines
+174
to
+177
| self._ensure_semaphore() | ||
| # No capacity limit → use original behaviour | ||
| if self._capacity_sem is None: | ||
| return await super().connect(env_config, seed=seed) |
Contributor
There was a problem hiding this comment.
Unlimited-capacity mode returns the wrong context type.
When capacity=0, this path stores a plain SessionContext via super().connect(). _handle_close(), _cleanup_loop(), and aclose() later read ctx._holds_slot unconditionally, so closing or timing out a session in unlimited mode will raise AttributeError before cleanup finishes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/handler.py` around lines 174 - 177, The
unlimited-capacity early-return in connect (the branch where self._capacity_sem
is None after calling self._ensure_semaphore and it returns await
super().connect(env_config, seed=seed)) yields a plain SessionContext missing
the _holds_slot attribute that _handle_close, _cleanup_loop, and aclose expect;
modify this path so the returned context is compatible by either wrapping the
super().connect() result into the same LimitedSessionContext (or whatever
subclass is used elsewhere) or set ctx._holds_slot = False on the returned
context before returning, ensuring the symbol connect() returns an object with
the _holds_slot attribute to avoid AttributeError during
_handle_close/_cleanup_loop/aclose.
Comment on lines
+203
to
+257
| asyncio.create_task(self._deferred_create(ctx)) | ||
|
|
||
| n_active = sum(1 for s in self._sessions.values() if s.env is not None) | ||
| n_queued = len(self._sessions) - n_active | ||
| # Estimate wait: (queued_ahead / capacity) * avg_episode_time | ||
| # Use a rough estimate of 15s per env creation cycle | ||
| estimated_wait = max(0, (n_queued - 1)) / max(1, self._capacity) * 15 | ||
|
|
||
| LOGGER.info( | ||
| f"[Handler] Session {session_id} queued " | ||
| f"(active={n_active}, queued={n_queued}, capacity={self._capacity}, " | ||
| f"est_wait={estimated_wait:.0f}s)" | ||
| ) | ||
|
|
||
| return HandlerResult(data={ | ||
| "session_id": session_id, | ||
| "status": "queued", | ||
| "estimated_wait_s": estimated_wait, | ||
| }) | ||
|
|
||
| async def _deferred_create(self, ctx: _DeferredSessionContext) -> None: | ||
| """Background task: acquire capacity slot, then create env. | ||
|
|
||
| Two-phase acquisition: | ||
| 1. capacity_sem – limits total running envs (held for env lifetime) | ||
| 2. startup_sem – limits concurrent Unity startups (held only during | ||
| EbAlfred.__init__, released as soon as the process | ||
| is running) | ||
| This prevents a "startup storm" when many capacity slots open at once. | ||
| """ | ||
| try: | ||
| LOGGER.info(f"[Handler] Session {ctx.session_id} waiting for capacity slot...") | ||
| await self._capacity_sem.acquire() | ||
| ctx._holds_slot = True | ||
| LOGGER.info(f"[Handler] Session {ctx.session_id} acquired capacity slot, waiting for startup slot...") | ||
|
|
||
| if self._startup_sem is not None: | ||
| await self._startup_sem.acquire() | ||
|
|
||
| LOGGER.info(f"[Handler] Session {ctx.session_id} starting Unity...") | ||
| try: | ||
| ctx.env = await self.create_env(ctx.env_config) | ||
| finally: | ||
| if self._startup_sem is not None: | ||
| self._startup_sem.release() | ||
|
|
||
| LOGGER.info(f"[Handler] Session {ctx.session_id} env ready") | ||
| except Exception as e: | ||
| LOGGER.error(f"[Handler] Session {ctx.session_id} env creation failed: {e}") | ||
| ctx._error = str(e) | ||
| if ctx._holds_slot: | ||
| self._capacity_sem.release() | ||
| ctx._holds_slot = False | ||
| finally: | ||
| ctx._ready.set() |
Contributor
There was a problem hiding this comment.
Queued sessions can leak Unity processes and capacity slots after close/timeout.
_deferred_create() is fire-and-forget and no reference is kept. If a queued session is closed or timed out before it becomes ready, this task keeps waiting, can still acquire the semaphores later, and may create an env for a session that has already been removed from self._sessions. That orphan env is never closed, and its capacity slot is never returned.
🛠️ Suggested direction
`@dataclass`
class _DeferredSessionContext(SessionContext):
"""SessionContext with extra fields for capacity-based deferred env creation."""
@@
_ready: Optional[asyncio.Event] = field(default=None, repr=False)
+ _create_task: Optional[asyncio.Task] = field(default=None, repr=False)
_holds_slot: bool = field(default=False, repr=False)
_error: Optional[str] = field(default=None, repr=False)
@@
- asyncio.create_task(self._deferred_create(ctx))
+ ctx._create_task = asyncio.create_task(self._deferred_create(ctx))Then cancel and await ctx._create_task from _handle_close(), _cleanup_loop(), and aclose(), and re-check that ctx.session_id is still present after each semaphore wait / after create_env() so orphaned envs are closed immediately.
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 203-203: Store a reference to the return value of asyncio.create_task
(RUF006)
[warning] 227-227: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 228-228: Docstring contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF002)
[warning] 250-250: Do not catch blind exception: Exception
(BLE001)
[warning] 251-251: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/handler.py` around lines 203 - 257, When spawning the
background task, store it on the session context (set ctx._create_task =
asyncio.create_task(self._deferred_create(ctx))) and ensure all close/cleanup
paths (_handle_close, _cleanup_loop, aclose) cancel and await ctx._create_task
before proceeding; inside _deferred_create, after each wait point (after
awaiting self._capacity_sem.acquire(), after acquiring self._startup_sem, and
after create_env returns) re-check that ctx.session_id is still present in
self._sessions and if it is not, immediately close the newly created env (if
any), release any held semaphores (self._startup_sem and/or self._capacity_sem)
and clear ctx._holds_slot; always set ctx._ready and ensure exceptions still set
ctx._error and release capacity slot if held to avoid leaking slots or Unity
processes.
Comment on lines
+163
to
+177
| def wm_format_prompt(max_actions_per_step, action_sep, add_example=True): | ||
| """Generate format prompt for wm format with observation and prediction tags.""" | ||
| base = f"""You should output {max_actions_per_step} action(s) at a time. | ||
| Output the action name exactly as listed in the available actions, or the action ID (integer). | ||
| Your response must be in the format of: | ||
| <observation>...</observation><think>...</think><answer>action name or action ID</answer><prediction>...</prediction>. | ||
|
|
||
| Rules for <observation>: | ||
| - Describe the current scene: what objects you see, your position, what you are holding, and relevant receptacle states. | ||
|
|
||
| Rules for <prediction>: | ||
| - Predict what will change after your action: where you will be, what you will see, and the expected result. | ||
|
|
||
| Rules for <answer>: | ||
| - Output exactly 1 action name or action ID.""" |
Contributor
There was a problem hiding this comment.
wm_format_prompt() contradicts multi-action mode.
When max_actions_per_step > 1, this prompt says to output multiple actions, but the <answer> rules still require exactly one action and never mention action_sep. parse_wm() does split multiple actions, so the prompt and parser currently disagree.
🛠️ Safer short-term fix
def wm_format_prompt(max_actions_per_step, action_sep, add_example=True):
"""Generate format prompt for wm format with observation and prediction tags."""
+ if max_actions_per_step != 1:
+ raise ValueError("wm prompt format currently supports exactly one action per step")
base = f"""You should output {max_actions_per_step} action(s) at a time.
Output the action name exactly as listed in the available actions, or the action ID (integer).
Your response must be in the format of:
<observation>...</observation><think>...</think><answer>action name or action ID</answer><prediction>...</prediction>.🧰 Tools
🪛 Ruff (0.15.4)
[warning] 163-163: Unused function argument: action_sep
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/utils/prompt.py` around lines 163 - 177, The
wm_format_prompt() text contradicts multi-action mode: update the prompt (inside
wm_format_prompt) so when max_actions_per_step > 1 it instructs the model to
output up to max_actions_per_step actions in the <answer> tag separated by the
provided action_sep; explicitly mention action_sep, allow multiple action names
or IDs (up to max_actions_per_step), and remove the “exactly 1 action” wording;
ensure the prompt aligns with parse_wm() behavior by describing the separator
and the allowed cardinality for <answer>.
YaningDylan
force-pushed
the
add-env-eb-alfred
branch
from
March 17, 2026 02:25
8372ffe to
d588c79
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vagen/envs_remote/service.py (1)
55-73:⚠️ Potential issue | 🟡 MinorUpdate the docstring to document the
max_inflightparameter.The docstring (lines 63-67) is incomplete—the
max_inflightparameter is missing from the Args section. Add documentation:Args: handler: Handler instance that implements environment logic + max_inflight: Maximum concurrent requests allowed (0 = unlimited, default: 0)All existing callers in the codebase (
serve.py,gpu_round_robin_handler.py,multiprocess_handler.py,simple_example.py) invokebuild_gym_service()without passingmax_inflight, defaulting to unlimited concurrency. This may be intentional, but the parameter should be documented so developers are aware it exists and can set it when needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs_remote/service.py` around lines 55 - 73, Update the build_gym_service docstring to include documentation for the max_inflight parameter: describe that max_inflight (int, default 0) controls the maximum number of concurrent in-flight requests allowed (0 meaning unlimited concurrency), explain its effect on request throttling/queueing for the FastAPI app returned by build_gym_service(handler, max_inflight), and note the default callers currently rely on (no explicit value -> unlimited). Reference the parameter name max_inflight and the function build_gym_service so readers can find and set it when needed.
🧹 Nitpick comments (2)
vagen/envs/eb_alfred/eb_alfred_env.py (1)
281-285: Prefix unused unpacked variables with underscore.Static analysis flags
obs_rawandstep_rewardas unused. Prefix with_to indicate intentional discard.📝 Suggested fix
- obs_raw, step_reward, step_done, step_info = ( + _obs_raw, _step_reward, step_done, step_info = ( await asyncio.wait_for( asyncio.to_thread(self.env.step, matched), timeout=60.0 ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/eb_alfred_env.py` around lines 281 - 285, The tuple assignment from awaiting asyncio.wait_for(asyncio.to_thread(self.env.step, matched), timeout=60.0) currently binds obs_raw and step_reward which are unused; rename those two bindings to _obs_raw and _step_reward to indicate intentional discard while keeping step_done and step_info as-is. Update the unpacking in the async call (the assignment that receives the result of asyncio.wait_for / asyncio.to_thread calling self.env.step) so static analysis no longer flags the unused variables.vagen/envs/eb_alfred/README.md (1)
162-165: Add language specifier to fenced code block.The expected output code block is missing a language identifier, which helps with syntax highlighting and accessibility.
📝 Suggested fix
-``` +```text Starting EB-ALFRED service on 0.0.0.0:8000 GPU displays: [:0, :1] (auto-balanced)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@vagen/envs/eb_alfred/README.mdaround lines 162 - 165, Update the fenced
code block containing the EB-ALFRED startup lines so it includes a language
specifier (e.g., "text") for proper syntax highlighting and accessibility;
replace the triple backticks around the block withtext before "Starting EB-ALFRED service on 0.0.0.0:8000" and close withafter "GPU displays: [:0,
:1] (auto-balanced)" to apply the change.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In@vagen/envs_remote/service.py:
- Around line 55-73: Update the build_gym_service docstring to include
documentation for the max_inflight parameter: describe that max_inflight (int,
default 0) controls the maximum number of concurrent in-flight requests allowed
(0 meaning unlimited concurrency), explain its effect on request
throttling/queueing for the FastAPI app returned by build_gym_service(handler,
max_inflight), and note the default callers currently rely on (no explicit value
-> unlimited). Reference the parameter name max_inflight and the function
build_gym_service so readers can find and set it when needed.
Nitpick comments:
In@vagen/envs/eb_alfred/eb_alfred_env.py:
- Around line 281-285: The tuple assignment from awaiting
asyncio.wait_for(asyncio.to_thread(self.env.step, matched), timeout=60.0)
currently binds obs_raw and step_reward which are unused; rename those two
bindings to _obs_raw and _step_reward to indicate intentional discard while
keeping step_done and step_info as-is. Update the unpacking in the async call
(the assignment that receives the result of asyncio.wait_for / asyncio.to_thread
calling self.env.step) so static analysis no longer flags the unused variables.In
@vagen/envs/eb_alfred/README.md:
- Around line 162-165: Update the fenced code block containing the EB-ALFRED
startup lines so it includes a language specifier (e.g., "text") for proper
syntax highlighting and accessibility; replace the triple backticks around the
block withtext before "Starting EB-ALFRED service on 0.0.0.0:8000" and close withafter "GPU displays: [:0, :1] (auto-balanced)" to apply the change.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `924b02c0-6edb-4005-9144-63864410fb52` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d588c7906c398b9ab986abc91a3522d8b71ccd88 and 1ad7eccee02fd5088ee869c6392979272e614961. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `vagen/envs/eb_alfred/README.md` * `vagen/envs/eb_alfred/eb_alfred_env.py` * `vagen/envs_remote/service.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
vagen/envs/eb_alfred/serve.py (1)
36-36:⚠️ Potential issue | 🟠 MajorDefault host still exposes service on all interfaces.
Line 36 keeps
0.0.0.0as default, which makes remote exposure opt-out instead of opt-in.Suggested change
- parser.add_argument("--host", type=str, default="0.0.0.0", help="Server host") + parser.add_argument("--host", type=str, default="127.0.0.1", help="Server host (use 0.0.0.0 only when intended)")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/serve.py` at line 36, Change the parser default to bind only to localhost so the service is opt-in for remote exposure: update the argparse declaration where parser.add_argument("--host", type=str, default="0.0.0.0", help="Server host") is defined to use "127.0.0.1" as the default and adjust the help text if needed to indicate remote binding must be explicitly requested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vagen/envs_remote/service.py`:
- Around line 40-47: The helper _read_data_field currently decodes uploaded
bytes with .decode("utf-8") which raises UnicodeDecodeError for non-UTF-8 input
and results in a 500; catch UnicodeDecodeError around the decode in
_read_data_field and raise fastapi.HTTPException(status_code=400,
detail="Invalid data encoding; expected UTF-8") so client errors produce 4xx.
Apply the same pattern to any other places that decode uploaded bytes directly
(the other byte-decoding occurrences noted in the review) to convert
UnicodeDecodeError into HTTPException(400).
In `@vagen/envs/eb_alfred/serve.py`:
- Around line 95-100: The startup hook _set_thread_pool will never run because
build_gym_service constructs the FastAPI app with a custom lifespan, so move the
thread-pool setup into that single lifecycle path: update the lifespan context
manager used by build_gym_service to call
asyncio.get_event_loop().set_default_executor(concurrent.futures.ThreadPoolExecutor(max_workers=_thread_workers))
during startup and shut down/cleanup the executor on shutdown (reference
symbols: _set_thread_pool, build_gym_service, _thread_workers,
ThreadPoolExecutor); remove the `@app.on_event`("startup") handler to avoid
duplicate lifecycle code.
In `@vagen/envs/eb_alfred/start_server.sh`:
- Around line 1-12: Add strict failure handling and cleanup: enable set -euo
pipefail at the top of start_server.sh, verify cp and Xorg commands succeed
(fail fast if any cp or Xorg background start fails), implement a cleanup
function that kills the Xorg background jobs (launched as Xorg ... :0 and :1)
and register it via trap on EXIT and signals, and add a readiness check (e.g.,
wait/poll for the Xorg displays or sockets for :0 and :1 to be available) before
launching python -m vagen.envs.eb_alfred.serve so the script waits for Xorg
readiness and doesn’t leave orphaned Xorg processes on error.
---
Duplicate comments:
In `@vagen/envs/eb_alfred/serve.py`:
- Line 36: Change the parser default to bind only to localhost so the service is
opt-in for remote exposure: update the argparse declaration where
parser.add_argument("--host", type=str, default="0.0.0.0", help="Server host")
is defined to use "127.0.0.1" as the default and adjust the help text if needed
to indicate remote binding must be explicitly requested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e147fb2-d432-4271-9df5-95ba7e42b054
📒 Files selected for processing (6)
vagen/envs/eb_alfred/README.mdvagen/envs/eb_alfred/serve.pyvagen/envs/eb_alfred/start_server.shvagen/envs/eb_alfred/xorg0.confvagen/envs/eb_alfred/xorg1.confvagen/envs_remote/service.py
✅ Files skipped from review due to trivial changes (1)
- vagen/envs/eb_alfred/xorg0.conf
🚧 Files skipped from review as they are similar to previous changes (1)
- vagen/envs/eb_alfred/README.md
Comment on lines
40
to
47
| async def _read_data_field(data: Union[str, UploadFile, None]) -> Optional[str]: | ||
| """Accept data field whether client sends it as plain string or as a file upload.""" | ||
| if data is None: | ||
| return None | ||
| if isinstance(data, str): | ||
| return data | ||
| return (await data.read()).decode("utf-8") | ||
|
|
Contributor
There was a problem hiding this comment.
Return 400 for invalid data encoding instead of 500.
If uploaded data bytes are not UTF-8, _read_data_field() raises UnicodeDecodeError, which currently falls into broad exception handlers and becomes a server error. This is client input and should be a 4xx.
Proposed fix
async def _read_data_field(data: Union[str, UploadFile, None]) -> Optional[str]:
"""Accept data field whether client sends it as plain string or as a file upload."""
if data is None:
return None
if isinstance(data, str):
return data
- return (await data.read()).decode("utf-8")
+ try:
+ return (await data.read()).decode("utf-8")
+ except UnicodeDecodeError:
+ raise HTTPException(status_code=400, detail="data must be UTF-8 text")Also applies to: 187-190, 258-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs_remote/service.py` around lines 40 - 47, The helper
_read_data_field currently decodes uploaded bytes with .decode("utf-8") which
raises UnicodeDecodeError for non-UTF-8 input and results in a 500; catch
UnicodeDecodeError around the decode in _read_data_field and raise
fastapi.HTTPException(status_code=400, detail="Invalid data encoding; expected
UTF-8") so client errors produce 4xx. Apply the same pattern to any other places
that decode uploaded bytes directly (the other byte-decoding occurrences noted
in the review) to convert UnicodeDecodeError into HTTPException(400).
Comment on lines
+1
to
+12
| #!/bin/bash | ||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
|
||
| cp "$SCRIPT_DIR/xorg0.conf" /tmp/xorg0.conf | ||
| cp "$SCRIPT_DIR/xorg1.conf" /tmp/xorg1.conf | ||
|
|
||
| Xorg -noreset +extension GLX -config /tmp/xorg0.conf :0 & | ||
| Xorg -noreset +extension GLX -config /tmp/xorg1.conf :1 & | ||
|
|
||
| sleep 2 | ||
|
|
||
| python -m vagen.envs.eb_alfred.serve \ |
Contributor
There was a problem hiding this comment.
Harden process lifecycle: fail fast, wait for readiness, and clean up Xorg children.
Current flow can continue after startup failures and can leave orphaned Xorg processes.
Proposed hardening
#!/bin/bash
+set -euo pipefail
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
@@
-Xorg -noreset +extension GLX -config /tmp/xorg0.conf :0 &
-Xorg -noreset +extension GLX -config /tmp/xorg1.conf :1 &
+Xorg -noreset +extension GLX -config /tmp/xorg0.conf :0 &
+X0_PID=$!
+Xorg -noreset +extension GLX -config /tmp/xorg1.conf :1 &
+X1_PID=$!
+
+cleanup() {
+ kill "$X0_PID" "$X1_PID" 2>/dev/null || true
+}
+trap cleanup EXIT INT TERM
-
-sleep 2
+# Wait until both displays are ready (max ~10s)
+for _ in {1..50}; do
+ [[ -S /tmp/.X11-unix/X0 && -S /tmp/.X11-unix/X1 ]] && break
+ sleep 0.2
+done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #!/bin/bash | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| cp "$SCRIPT_DIR/xorg0.conf" /tmp/xorg0.conf | |
| cp "$SCRIPT_DIR/xorg1.conf" /tmp/xorg1.conf | |
| Xorg -noreset +extension GLX -config /tmp/xorg0.conf :0 & | |
| Xorg -noreset +extension GLX -config /tmp/xorg1.conf :1 & | |
| sleep 2 | |
| python -m vagen.envs.eb_alfred.serve \ | |
| #!/bin/bash | |
| set -euo pipefail | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| cp "$SCRIPT_DIR/xorg0.conf" /tmp/xorg0.conf | |
| cp "$SCRIPT_DIR/xorg1.conf" /tmp/xorg1.conf | |
| Xorg -noreset +extension GLX -config /tmp/xorg0.conf :0 & | |
| X0_PID=$! | |
| Xorg -noreset +extension GLX -config /tmp/xorg1.conf :1 & | |
| X1_PID=$! | |
| cleanup() { | |
| kill "$X0_PID" "$X1_PID" 2>/dev/null || true | |
| } | |
| trap cleanup EXIT INT TERM | |
| # Wait until both displays are ready (max ~10s) | |
| for _ in {1..50}; do | |
| [[ -S /tmp/.X11-unix/X0 && -S /tmp/.X11-unix/X1 ]] && break | |
| sleep 0.2 | |
| done | |
| python -m vagen.envs.eb_alfred.serve \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/start_server.sh` around lines 1 - 12, Add strict failure
handling and cleanup: enable set -euo pipefail at the top of start_server.sh,
verify cp and Xorg commands succeed (fail fast if any cp or Xorg background
start fails), implement a cleanup function that kills the Xorg background jobs
(launched as Xorg ... :0 and :1) and register it via trap on EXIT and signals,
and add a readiness check (e.g., wait/poll for the Xorg displays or sockets for
:0 and :1 to be available) before launching python -m vagen.envs.eb_alfred.serve
so the script waits for Xorg readiness and doesn’t leave orphaned Xorg processes
on error.
…meout; fix envs_remote aclose parallel shutdown
main refactored envs_remote/service.py from build_gym_service() function to GymService class; update serve.py accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Mirror navigation's _detect_gpus() (CUDA_VISIBLE_DEVICES or nvidia-smi) - Auto-generate /tmp/xorgN.conf from nvidia-smi PCI bus IDs (hex→decimal) - Auto-start Xorg :N for each GPU N, skip if already running - Switch from argparse to fire.Fire (consistent with navigation) - Rename --x-displays to --devices (List[int], consistent with navigation) - Remove hardcoded xorg0.conf / xorg1.conf (machine-specific BusIDs) - Simplify start_server.sh to a one-liner example Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Concise sections: Running Service / Evaluation / Training / Prompt Formats / Datasets / Interactive Test / Checklist. Remove verbose prose, add auto-detect GPU/Xorg note, SSH tunnel tip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, drop Interactive Test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
YaningDylan
force-pushed
the
add-env-eb-alfred
branch
from
March 18, 2026 02:08
a2d93fd to
ef9dd73
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
vagen/envs/eb_alfred/utils/utils.py (1)
150-154:⚠️ Potential issue | 🟡 MinorValidate HWC shape before calling
Image.fromarray.
shape[-1] == 3also accepts non-image arrays like(3, N)or 4-D tensors, so the real failure gets deferred to PIL. Checkndim == 3 and shape[2] == 3here and raise a clearValueErrorwhen the layout is wrong.Proposed fix
def numpy_to_pil(numpy_array: np.ndarray) -> Image.Image: """Convert numpy (H, W, 3) to PIL.Image in RGB.""" - if numpy_array.shape[-1] == 3: + if numpy_array.ndim == 3 and numpy_array.shape[2] == 3: return Image.fromarray(numpy_array.astype(np.uint8), mode="RGB") raise ValueError(f"Unsupported channels: {numpy_array.shape[-1]}. Expected 3 (RGB).")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/utils/utils.py` around lines 150 - 154, The numpy_to_pil function currently only checks shape[-1]==3 which admits non-HWC arrays; update it to validate the array is 3-dimensional and in HWC layout by checking numpy_array.ndim == 3 and numpy_array.shape[2] == 3, and if not raise a clear ValueError (e.g., "Expected HWC numpy array with 3 channels, got shape ...") before calling Image.fromarray; keep the existing dtype cast and mode="RGB" behavior when the validation passes.vagen/envs/eb_alfred/serve.py (2)
139-160:⚠️ Potential issue | 🟠 MajorDefault to loopback unless remote exposure is explicitly requested.
With
host="0.0.0.0"andapi_key="", a plainpython -m vagen.envs.eb_alfred.serveexposes unauthenticatedconnect/callendpoints on every interface. Safer default is loopback, with remote binding as an explicit opt-in.Proposed fix
-def main( - host: str = "0.0.0.0", +def main( + host: str = "127.0.0.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/serve.py` around lines 139 - 160, Change the default binding in the main(...) function to loopback so the server does not expose unauthenticated endpoints by default: update the host parameter default from "0.0.0.0" to "127.0.0.1" in the main signature and adjust any related docstring/comment that mentions binding behavior; leave the api_key parameter behavior unchanged so remote exposure remains an explicit opt-in (users can still pass host="0.0.0.0" and/or set api_key to enable remote access).
186-194:⚠️ Potential issue | 🟠 MajorMove executor setup/teardown into the existing lifespan context.
GymService.build()already creates the FastAPI app with a custom lifespan invagen/envs_remote/service.py:244-268, so these@app.on_event(...)hooks will be skipped. That leavesthread_pool_sizeineffective and the custom executor never gets shut down.Possible fix
+from contextlib import asynccontextmanager ... handler = EbAlfredHandler( x_displays=x_displays, capacity=capacity, startup_concurrency=startup_concurrency, session_timeout=session_timeout, max_sessions=max_sessions, ) app = GymService(handler, api_key=api_key).build() - - `@app.on_event`("startup") - async def _configure_executor(): - asyncio.get_running_loop().set_default_executor(executor) - - `@app.on_event`("shutdown") - def _shutdown_executor(): - executor.shutdown(wait=True) + base_lifespan = app.router.lifespan_context + + `@asynccontextmanager` + async def _lifespan(app_): + asyncio.get_running_loop().set_default_executor(executor) + async with base_lifespan(app_): + yield + executor.shutdown(wait=True) + + app.router.lifespan_context = _lifespan🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/serve.py` around lines 186 - 194, The executor setup/teardown done by the `@app.on_event` handlers (_configure_executor and _shutdown_executor) is skipped because GymService.build() supplies a custom lifespan; move the executor creation and executor.shutdown(wait=True) into that existing lifespan context so thread_pool_size takes effect and the executor is closed. Concretely, locate the lifespan generator used by GymService.build() (the lifespan function referenced in the build implementation) and: create/set the default executor (asyncio.get_running_loop().set_default_executor(executor)) at startup inside that lifespan before yielding, and call executor.shutdown(wait=True) after the yield; ensure any references to thread_pool_size/executor from the outer scope are passed into or accessible from the lifespan.vagen/envs/eb_alfred/handler.py (1)
288-307:⚠️ Potential issue | 🟡 MinorGuard
_holds_slotaccess for compatibility with unlimited mode.This method unconditionally accesses
ctx._holds_slotat line 296, but sessions created in unlimited mode (viasuper().connect()) don't have this attribute. Usegetattr(ctx, '_holds_slot', False)for defensive access.🔧 Suggested fix
finally: - if ctx._holds_slot and self._capacity_sem is not None: + if getattr(ctx, '_holds_slot', False) and self._capacity_sem is not None: self._capacity_sem.release() - ctx._holds_slot = False + ctx._holds_slot = False # type: ignore[union-attr] self._sessions.pop(ctx.session_id, None)Apply the same pattern to
_cleanup_loop()(line 344) andaclose()(line 369).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/handler.py` around lines 288 - 307, The method _handle_close unconditionally accesses ctx._holds_slot which breaks when sessions from unlimited mode (created via super().connect()) lack that attribute; change accesses like "if ctx._holds_slot and self._capacity_sem is not None" to use getattr(ctx, '_holds_slot', False) so it safely defaults to False, and likewise guard any writes/assignments with setattr only when appropriate (or use setattr(ctx, '_holds_slot', False) if you need to ensure the attribute exists). Apply the same defensive pattern in the _cleanup_loop and aclose methods where ctx._holds_slot is read or modified, and keep the existing logic that releases self._capacity_sem and sets ctx._holds_slot = False only when getattr(ctx, '_holds_slot', False) is True and self._capacity_sem is not None.
🧹 Nitpick comments (2)
vagen/envs/eb_alfred/eb_alfred_env.py (2)
281-285: Prefix unused unpacked variables with underscore.
obs_rawandstep_rewardare unpacked but never used. Prefix them with_to indicate they are intentionally ignored and silence the linter warning.✨ Suggested fix
- obs_raw, step_reward, step_done, step_info = ( + _obs_raw, _step_reward, step_done, step_info = ( await asyncio.wait_for( asyncio.to_thread(self.env.step, matched), timeout=60.0 ) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/eb_alfred_env.py` around lines 281 - 285, Unpack the tuple returned from awaiting asyncio.wait_for(asyncio.to_thread(self.env.step, matched), ...) using underscore-prefixed names for the unused values: replace obs_raw and step_reward with _obs_raw and _step_reward while keeping step_done and step_info as-is so the linter knows those first two are intentionally ignored in eb_alfred_env.py where self.env.step is invoked inside asyncio.wait_for/asyncio.to_thread.
195-196: Direct manipulation of internal_current_episode_numis fragile.Setting
self.env._current_episode_numdirectly accesses internal state ofEBAlfEnv. This could break if the upstream library changes its implementation. Consider checking ifEBAlfEnvprovides a public API for episode selection, or document this as a known coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vagen/envs/eb_alfred/eb_alfred_env.py` around lines 195 - 196, The code directly assigns self.env._current_episode_num which mutates EBAlfEnv's internal state; instead, check for and call a public API on EBAlfEnv to set the episode (e.g., a method like set_current_episode, select_episode, or reset/seed that accepts an episode index) using episode_idx = seed % self.env.number_of_episodes; if no public API exists, wrap the internal assignment behind a clearly documented helper (e.g., a method on this wrapper) and guard it with a try/except or hasattr check before using setattr(self.env, "_current_episode_num", episode_idx) so the coupling is explicit and fails safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/train/eb_alfred/train_eb_alfred_vision.yaml`:
- Around line 2-66: The YAML includes an unsupported field tag_id which causes
EnvSpec construction to raise a TypeError because EnvSpec (instantiated via
EnvSpec(**OmegaConf.to_container(...)) in the loader) doesn’t define tag_id; fix
by either removing/moving tag_id from each RemoteEnv entry (e.g., put it inside
config or another supported field) or add tag_id to the EnvSpec dataclass
definition so EnvSpec accepts it—update the EnvSpec dataclass (and any related
type annotations/serializers) or relocate tag_id in the YAML accordingly.
In `@examples/train/eb_alfred/train_ppo_no_concat_qwen25vl3b.sh`:
- Around line 8-15: The script mixes PWD-based paths with script-relative paths
so running it outside the repo root breaks config resolution; replace usages
that derive from BASEDIR=$(pwd) (notably EXPERIMENT_DIR, SAVE_CHECKPOINT_DIR,
and agent_loop_config_path / any --config-path references) with paths computed
relative to the script location (use SCRIPTDIR=$(dirname "$0") and derive a repo
root or scripts root from that) so DATASET_TRAIN, DATASET_VAL,
agent_loop_config_path, and SAVE_CHECKPOINT_DIR all point to files under the
repository regardless of the caller's working directory.
- Around line 1-4: The script enables only set -x so pipeline failures can be
masked; update the shell initialization in train_ppo_no_concat_qwen25vl3b.sh to
enable strict failure handling (set -euo pipefail) before the pipeline that runs
python3 -m vagen.main_ppo ... | tee ..., and apply the same change to the other
invocation around lines 85-86; ensure the strict flags are declared at the top
of the script so any failed command in the pipeline causes the script to exit
nonzero.
In `@examples/train/eb_alfred/val_eb_alfred_vision.yaml`:
- Around line 2-44: The YAML uses an unsupported field tag_id which causes
EnvSpec construction to fail; update the dataclass EnvSpec (in
vagen/gym_agent_dataset.py) to include a tag_id: str | None field (and default
to None) or move tag_id into an existing nested config field (e.g., under
config) and update load_envspecs() to map the YAML key accordingly; ensure
EnvSpec's __init__/type hints accept the new field and any downstream code that
references EnvSpec (or load_envspecs()) is adjusted to read tag_id from the new
location.
In `@vagen/envs/eb_alfred/start_server.sh`:
- Around line 5-8: The wrapper script start_server.sh currently invokes the
serve module without forwarding caller overrides and should be changed to use
exec to replace the shell and pass through all arguments; update the invocation
of python -m vagen.envs.eb_alfred.serve to use exec and include "$@" so flags
like --devices or custom --port/--capacity are forwarded to the Python process
and signals are handled by Python directly.
---
Duplicate comments:
In `@vagen/envs/eb_alfred/handler.py`:
- Around line 288-307: The method _handle_close unconditionally accesses
ctx._holds_slot which breaks when sessions from unlimited mode (created via
super().connect()) lack that attribute; change accesses like "if ctx._holds_slot
and self._capacity_sem is not None" to use getattr(ctx, '_holds_slot', False) so
it safely defaults to False, and likewise guard any writes/assignments with
setattr only when appropriate (or use setattr(ctx, '_holds_slot', False) if you
need to ensure the attribute exists). Apply the same defensive pattern in the
_cleanup_loop and aclose methods where ctx._holds_slot is read or modified, and
keep the existing logic that releases self._capacity_sem and sets
ctx._holds_slot = False only when getattr(ctx, '_holds_slot', False) is True and
self._capacity_sem is not None.
In `@vagen/envs/eb_alfred/serve.py`:
- Around line 139-160: Change the default binding in the main(...) function to
loopback so the server does not expose unauthenticated endpoints by default:
update the host parameter default from "0.0.0.0" to "127.0.0.1" in the main
signature and adjust any related docstring/comment that mentions binding
behavior; leave the api_key parameter behavior unchanged so remote exposure
remains an explicit opt-in (users can still pass host="0.0.0.0" and/or set
api_key to enable remote access).
- Around line 186-194: The executor setup/teardown done by the `@app.on_event`
handlers (_configure_executor and _shutdown_executor) is skipped because
GymService.build() supplies a custom lifespan; move the executor creation and
executor.shutdown(wait=True) into that existing lifespan context so
thread_pool_size takes effect and the executor is closed. Concretely, locate the
lifespan generator used by GymService.build() (the lifespan function referenced
in the build implementation) and: create/set the default executor
(asyncio.get_running_loop().set_default_executor(executor)) at startup inside
that lifespan before yielding, and call executor.shutdown(wait=True) after the
yield; ensure any references to thread_pool_size/executor from the outer scope
are passed into or accessible from the lifespan.
In `@vagen/envs/eb_alfred/utils/utils.py`:
- Around line 150-154: The numpy_to_pil function currently only checks
shape[-1]==3 which admits non-HWC arrays; update it to validate the array is
3-dimensional and in HWC layout by checking numpy_array.ndim == 3 and
numpy_array.shape[2] == 3, and if not raise a clear ValueError (e.g., "Expected
HWC numpy array with 3 channels, got shape ...") before calling Image.fromarray;
keep the existing dtype cast and mode="RGB" behavior when the validation passes.
---
Nitpick comments:
In `@vagen/envs/eb_alfred/eb_alfred_env.py`:
- Around line 281-285: Unpack the tuple returned from awaiting
asyncio.wait_for(asyncio.to_thread(self.env.step, matched), ...) using
underscore-prefixed names for the unused values: replace obs_raw and step_reward
with _obs_raw and _step_reward while keeping step_done and step_info as-is so
the linter knows those first two are intentionally ignored in eb_alfred_env.py
where self.env.step is invoked inside asyncio.wait_for/asyncio.to_thread.
- Around line 195-196: The code directly assigns self.env._current_episode_num
which mutates EBAlfEnv's internal state; instead, check for and call a public
API on EBAlfEnv to set the episode (e.g., a method like set_current_episode,
select_episode, or reset/seed that accepts an episode index) using episode_idx =
seed % self.env.number_of_episodes; if no public API exists, wrap the internal
assignment behind a clearly documented helper (e.g., a method on this wrapper)
and guard it with a try/except or hasattr check before using setattr(self.env,
"_current_episode_num", episode_idx) so the coupling is explicit and fails
safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35e5e5c1-e27c-48fb-bbe8-51d59d1f0375
📒 Files selected for processing (14)
examples/evaluate/eb_alfred/config.yamlexamples/evaluate/eb_alfred/run_eval.shexamples/train/eb_alfred/train_eb_alfred_vision.yamlexamples/train/eb_alfred/train_ppo_no_concat_qwen25vl3b.shexamples/train/eb_alfred/val_eb_alfred_vision.yamlvagen/envs/eb_alfred/README.mdvagen/envs/eb_alfred/__init__.pyvagen/envs/eb_alfred/eb_alfred_env.pyvagen/envs/eb_alfred/handler.pyvagen/envs/eb_alfred/serve.pyvagen/envs/eb_alfred/start_server.shvagen/envs/eb_alfred/utils/__init__.pyvagen/envs/eb_alfred/utils/prompt.pyvagen/envs/eb_alfred/utils/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- vagen/envs/eb_alfred/utils/prompt.py
- examples/evaluate/eb_alfred/config.yaml
Comment on lines
+2
to
+66
| - name: RemoteEnv | ||
| n_envs: 50 | ||
| data_source: eb_alfred | ||
| tag_id: eb_alfred_train_base | ||
| seed: [0, 200, 1] | ||
| max_turns: 6 | ||
| response_length_per_turn: 512 | ||
| config: | ||
| base_urls: | ||
| - "http://localhost:8000" | ||
| timeout: 600 | ||
| eval_set: base | ||
| obs_image_size: 500 | ||
| max_turns: 6 | ||
| max_actions_per_step: 20 | ||
| max_env_steps: 30 | ||
| action_sep: "," | ||
| prompt_format: free_think | ||
| use_example_in_sys_prompt: true | ||
| format_reward: 0.1 | ||
| success_reward: 1.0 | ||
|
|
||
| - name: RemoteEnv | ||
| n_envs: 50 | ||
| data_source: eb_alfred | ||
| tag_id: eb_alfred_train_complex | ||
| seed: [0, 200, 1] | ||
| max_turns: 6 | ||
| response_length_per_turn: 512 | ||
| config: | ||
| base_urls: | ||
| - "http://localhost:8000" | ||
| timeout: 600 | ||
| eval_set: complex_instruction | ||
| obs_image_size: 500 | ||
| max_turns: 6 | ||
| max_actions_per_step: 20 | ||
| max_env_steps: 30 | ||
| action_sep: "," | ||
| prompt_format: free_think | ||
| use_example_in_sys_prompt: true | ||
| format_reward: 0.1 | ||
| success_reward: 1.0 | ||
|
|
||
| - name: RemoteEnv | ||
| n_envs: 50 | ||
| data_source: eb_alfred | ||
| tag_id: eb_alfred_train_visual | ||
| seed: [0, 200, 1] | ||
| max_turns: 6 | ||
| response_length_per_turn: 512 | ||
| config: | ||
| base_urls: | ||
| - "http://localhost:8000" | ||
| timeout: 600 | ||
| eval_set: visual_appearance | ||
| obs_image_size: 500 | ||
| max_turns: 6 | ||
| max_actions_per_step: 20 | ||
| max_env_steps: 30 | ||
| action_sep: "," | ||
| prompt_format: free_think | ||
| use_example_in_sys_prompt: true | ||
| format_reward: 0.1 | ||
| success_reward: 1.0 |
Contributor
There was a problem hiding this comment.
tag_id is not accepted by EnvSpec here either.
The env-spec loader in vagen/gym_agent_dataset.py:15-43 instantiates each entry with EnvSpec(**OmegaConf.to_container(...)), and that dataclass does not define tag_id. Training will fail with TypeError before the dataset is even built unless this field is added to the schema or moved to a supported location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/train/eb_alfred/train_eb_alfred_vision.yaml` around lines 2 - 66,
The YAML includes an unsupported field tag_id which causes EnvSpec construction
to raise a TypeError because EnvSpec (instantiated via
EnvSpec(**OmegaConf.to_container(...)) in the loader) doesn’t define tag_id; fix
by either removing/moving tag_id from each RemoteEnv entry (e.g., put it inside
config or another supported field) or add tag_id to the EnvSpec dataclass
definition so EnvSpec accepts it—update the EnvSpec dataclass (and any related
type annotations/serializers) or relocate tag_id in the YAML accordingly.
Comment on lines
+1
to
+4
| #!/bin/bash | ||
|
|
||
| set -x | ||
|
|
Contributor
There was a problem hiding this comment.
Enable -euo pipefail before piping trainer output to tee.
As written, python3 -m vagen.main_ppo ... | tee ... can fail while the script still exits 0, because only set -x is enabled. That makes failed runs easy to miss in schedulers and CI.
Proposed fix
-#!/bin/bash
-
-set -x
+#!/bin/bash
+set -euo pipefail
+set -xAlso applies to: 85-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/train/eb_alfred/train_ppo_no_concat_qwen25vl3b.sh` around lines 1 -
4, The script enables only set -x so pipeline failures can be masked; update the
shell initialization in train_ppo_no_concat_qwen25vl3b.sh to enable strict
failure handling (set -euo pipefail) before the pipeline that runs python3 -m
vagen.main_ppo ... | tee ..., and apply the same change to the other invocation
around lines 85-86; ensure the strict flags are declared at the top of the
script so any failed command in the pipeline causes the script to exit nonzero.
Comment on lines
+8
to
+15
| BASEDIR=$(pwd) | ||
| SCRIPTDIR=$(dirname "$0") | ||
| EXPERIMENT_DIR=${BASEDIR}/exps/${PROJECT_NAME}/${EXPERIMENT_NAME} | ||
| SAVE_CHECKPOINT_DIR=${EXPERIMENT_DIR}/verl_checkpoints | ||
| DATASET_TRAIN=${SCRIPTDIR}/train_eb_alfred_vision.yaml | ||
| DATASET_VAL=${SCRIPTDIR}/val_eb_alfred_vision.yaml | ||
| agent_loop_config_path=${BASEDIR}/vagen/configs/agent_no_concat.yaml | ||
| REF_MODEL_PATH=Qwen/Qwen2.5-VL-3B-Instruct |
Contributor
There was a problem hiding this comment.
Stop deriving repo paths from the caller's working directory.
DATASET_TRAIN and DATASET_VAL are anchored to the script location, but --config-path and agent_loop_config_path are anchored to $PWD. Running this launcher from anywhere but the repo root points those paths at the wrong files.
Also applies to: 21-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/train/eb_alfred/train_ppo_no_concat_qwen25vl3b.sh` around lines 8 -
15, The script mixes PWD-based paths with script-relative paths so running it
outside the repo root breaks config resolution; replace usages that derive from
BASEDIR=$(pwd) (notably EXPERIMENT_DIR, SAVE_CHECKPOINT_DIR, and
agent_loop_config_path / any --config-path references) with paths computed
relative to the script location (use SCRIPTDIR=$(dirname "$0") and derive a repo
root or scripts root from that) so DATASET_TRAIN, DATASET_VAL,
agent_loop_config_path, and SAVE_CHECKPOINT_DIR all point to files under the
repository regardless of the caller's working directory.
Comment on lines
+2
to
+44
| - name: RemoteEnv | ||
| n_envs: 50 | ||
| data_source: eb_alfred | ||
| tag_id: eb_alfred_val_common | ||
| seed: [0, 50, 1] | ||
| max_turns: 6 | ||
| response_length_per_turn: 512 | ||
| config: | ||
| base_urls: | ||
| - "http://localhost:8000" | ||
| timeout: 600 | ||
| eval_set: common_sense | ||
| obs_image_size: 500 | ||
| max_turns: 6 | ||
| max_actions_per_step: 20 | ||
| max_env_steps: 30 | ||
| action_sep: "," | ||
| prompt_format: free_think | ||
| use_example_in_sys_prompt: true | ||
| format_reward: 0.1 | ||
| success_reward: 1.0 | ||
|
|
||
| - name: RemoteEnv | ||
| n_envs: 50 | ||
| data_source: eb_alfred | ||
| tag_id: eb_alfred_val_spatial | ||
| seed: [0, 50, 1] | ||
| max_turns: 6 | ||
| response_length_per_turn: 512 | ||
| config: | ||
| base_urls: | ||
| - "http://localhost:8000" | ||
| timeout: 600 | ||
| eval_set: spatial | ||
| obs_image_size: 500 | ||
| max_turns: 6 | ||
| max_actions_per_step: 20 | ||
| max_env_steps: 30 | ||
| action_sep: "," | ||
| prompt_format: free_think | ||
| use_example_in_sys_prompt: true | ||
| format_reward: 0.1 | ||
| success_reward: 1.0 |
Contributor
There was a problem hiding this comment.
tag_id is not part of the EnvSpec schema.
load_envspecs() constructs each YAML entry as EnvSpec(**OmegaConf.to_container(...)), and the EnvSpec dataclass shown in vagen/gym_agent_dataset.py:15-43 has no tag_id field. This config will currently raise TypeError before any validation episode starts. Either add tag_id to EnvSpec or move it into a supported nested config field.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/train/eb_alfred/val_eb_alfred_vision.yaml` around lines 2 - 44, The
YAML uses an unsupported field tag_id which causes EnvSpec construction to fail;
update the dataclass EnvSpec (in vagen/gym_agent_dataset.py) to include a
tag_id: str | None field (and default to None) or move tag_id into an existing
nested config field (e.g., under config) and update load_envspecs() to map the
YAML key accordingly; ensure EnvSpec's __init__/type hints accept the new field
and any downstream code that references EnvSpec (or load_envspecs()) is adjusted
to read tag_id from the new location.
Comment on lines
+5
to
+8
| python -m vagen.envs.eb_alfred.serve \ | ||
| --port 8000 \ | ||
| --capacity 90 \ | ||
| --startup_concurrency 6 |
Contributor
There was a problem hiding this comment.
Forward user overrides through the wrapper script.
The comment on Line 4 says callers can pass --devices='[0,1]', but this wrapper never forwards "$@", so every override is silently ignored. Using exec here also gives the Python process direct signal handling.
Proposed fix
-python -m vagen.envs.eb_alfred.serve \
+exec python -m vagen.envs.eb_alfred.serve \
--port 8000 \
--capacity 90 \
- --startup_concurrency 6
+ --startup_concurrency 6 \
+ "$@"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| python -m vagen.envs.eb_alfred.serve \ | |
| --port 8000 \ | |
| --capacity 90 \ | |
| --startup_concurrency 6 | |
| exec python -m vagen.envs.eb_alfred.serve \ | |
| --port 8000 \ | |
| --capacity 90 \ | |
| --startup_concurrency 6 \ | |
| "$@" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vagen/envs/eb_alfred/start_server.sh` around lines 5 - 8, The wrapper script
start_server.sh currently invokes the serve module without forwarding caller
overrides and should be changed to use exec to replace the shell and pass
through all arguments; update the invocation of python -m
vagen.envs.eb_alfred.serve to use exec and include "$@" so flags like --devices
or custom --port/--capacity are forwarded to the Python process and signals are
handled by Python directly.
Change all prompt examples, available actions list, and format instructions from `action_name (id: N)` to `[N, 'action_name']` to match ERA SFT data format exactly. Update match_action parser to handle quoted names. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
YaningDylan
force-pushed
the
add-env-eb-alfred
branch
from
March 25, 2026 00:40
7bf5ed5 to
192fa72
Compare
When n_gpus_per_node <= 2, call sleep()/wake_up() on the actor_rollout worker group before and after critic/actor updates to release SGLang's KV cache and weight memory. This prevents OOM during update_critic on memory-constrained setups (e.g. 2x H200 with large prompts). 4+ GPU configs are unaffected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When env.reset() throws an exception (e.g. server 500, timeout), return a dummy AgentLoopOutput with reward=0 instead of crashing the entire training run. Includes a dummy image to keep batch consistency with image_data fields. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation