Skip to content

Fix uv streamlit test - #1078

Draft
qianl-nv wants to merge 3 commits into
mainfrom
qianl/fix/uv-streamlit-test
Draft

Fix uv streamlit test#1078
qianl-nv wants to merge 3 commits into
mainfrom
qianl/fix/uv-streamlit-test

Conversation

@qianl-nv

Copy link
Copy Markdown
Collaborator

No description provided.

Streamlit is an optional dep; without --extra dev, collecting test_review_gui.py fails before Newton marker filtering.

Signed-off-by: Qian Lin <qianl@nvidia.com>
Kit UI fails under native uv; the sim-preview subprocess test now disables the visualizer while the GUI runner still defaults to Kit.

Signed-off-by: Qian Lin <qianl@nvidia.com>
@greptile-apps

greptile-apps Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes the review-GUI simulation preview test runnable in native uv CI by installing development extras, moving the test into the CI-collected core suite, and adding a headless camera-enabled SimApp launch mode.

  • Adds the dev optional dependency extra to native uv synchronization.
  • Moves the review-GUI tests into isaaclab_arena/tests.
  • Propagates a headless launch option from the client through the SimApp server and boot configuration.
  • Keeps interactive GUI launches on the existing Kit visualizer path by default.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking defects identified.

The new CI dependency flag references an existing optional extra, the interactive visualizer remains the default, and the test-only headless selection is consistently propagated through the client, server, and boot layers.

Important Files Changed

Filename Overview
.github/workflows/ci.yml Installs the valid dev optional-dependency extra so Streamlit-backed tests can be collected in both native uv matrix variants.
isaaclab_arena/tests/test_review_gui.py Moves the review-GUI suite into the CI-collected core test tree and selects headless mode for its SimApp subprocess test.
isaaclab_arena_examples/agentic_environment_generation/review_gui/simapp/boot.py Adds a camera-enabled headless launch configuration while preserving the Kit visualizer as the default.
isaaclab_arena_examples/agentic_environment_generation/review_gui/simapp/client.py Adds an opt-in visualizer flag and passes a static --headless argument to the server subprocess.
isaaclab_arena_examples/agentic_environment_generation/review_gui/simapp/server.py Parses the headless command-line option and consistently forwards the selected launch mode to SimApp boot.

Sequence Diagram

sequenceDiagram
  participant Test as Review GUI test
  participant Client as SimApp client
  participant Server as SimApp server
  participant Boot as SimApp boot
  participant Isaac as Isaac Sim
  Test->>Client: "spawn(enable_visualizer=False)"
  Client->>Server: launch --headless
  Server->>Boot: "launch_simulation_app(enable_visualizer=False)"
  Boot->>Isaac: "headless=True, enable_cameras=True"
  Isaac-->>Server: SimulationApp
  Test->>Server: preview request over Unix socket
  Server-->>Test: preview response
Loading

Reviews (1): Last reviewed commit: "Allow review GUI SimApp to boot headless..." | Re-trigger Greptile

Comment thread .github/workflows/ci.yml
# Fix dubious ownership issues in cache
git config --global --add safe.directory '*'
uv sync ${{ matrix.uv_flags }}
uv sync ${{ matrix.uv_flags }} --extra dev

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.

🟡 Will this break the Docker test steps too?

Moving test_review_gui.py into isaaclab_arena/tests/ means every step that collects that directory now imports streamlit at module top (via streamlit_ui). This --extra dev covers the native-uv job, but the Docker steps install with pip install -e . and no dev extra — and with no continue-on-collection-errors, a missing streamlit fails the whole phase at collection, not just this one test.

Does the isaaclab_arena-latest image already bundle streamlit? If not, would it be cleaner to keep the test in isaaclab_arena_examples/tests/ and add that directory to the CI pytest calls, so a dev-only, examples-testing test doesn't become a hard dependency of the core suite?

@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

Fixes CI collection of the review-GUI test suite: syncs the package's dev extra in the native-uv job (so optional streamlit is present), re-enables the test by moving it into isaaclab_arena/tests/, and lets the sim-preview subprocess boot headless-with-cameras so Kit UI isn't required under native uv. The functional change — threading enable_visualizer through spawn_simapp_process--headless_serve_socketlaunch_simulation_applaunch_args — is consistent and correct, and the subprocess test keeps its with_subprocess marker. One design/CI concern below.

Design, Boundaries & Scope

Relocating test_review_gui.py into isaaclab_arena/tests/ pulls a dev-only, examples-testing test into the core test suite: it imports isaaclab_arena_examples.* and streamlit (at module top via streamlit_ui). Because there's no continue-on-collection-errors, any step collecting isaaclab_arena/tests/ without streamlit now fails the whole phase at collection. The uv job got --extra dev, but the Docker steps install pip install -e . with no dev extra — so this only holds if the isaaclab_arena-latest image already bundles streamlit. Keeping the test in isaaclab_arena_examples/tests/ and adding that directory to the CI pytest calls would run it without inverting the core→examples dependency (see inline comment).

Findings

🟡 Warning: .github/workflows/ci.yml:442 — The moved test makes streamlit a collection-time dependency of the entire isaaclab_arena/tests/ directory; only the uv job installs the dev extra, so the Docker steps may fail collection unless the image already has streamlit.

Test Coverage

No new product logic — this is a test-relocation + CI fix. The subprocess test correctly carries @pytest.mark.with_subprocess and runs the sim in a separate process (the subprocess pattern, not the in-process inner/outer pattern), so markers and phase placement are right.

Verdict

Minor fixes needed — confirm the Docker test steps have streamlit (or run the test from the examples tests dir) before relying on this in CI.

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