Skip to content

Add Plotly runtime dependency - #1212

Open
zhx06 wants to merge 1 commit into
mainfrom
zxiao/fix/add-plotly-dependency
Open

Add Plotly runtime dependency#1212
zhx06 wants to merge 1 commit into
mainfrom
zxiao/fix/add-plotly-dependency

Conversation

@zhx06

@zhx06 zhx06 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Declare Plotly as a runtime dependency for the relation solver visualization examples.

  • Update pyproject.toml and uv.lock.
  • Fix ModuleNotFoundError: No module named 'plotly' in the dummy ObjectPlacer example.\n- Verify the example completes successfully in Docker.

Signed-off-by: zhx06 <zihaox@nvidia.com>
@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; the dependency declaration and lock metadata are consistent.

Summary

  • Adds Plotly to the project dependency list.
  • Updates the root package metadata in uv.lock.
  • Reuses the existing locked Plotly 6.9.0 package and transitive dependencies.

Comment thread pyproject.toml
"sbi",
"scipy",
"matplotlib",
"plotly",

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.

🔵 Worth saying why plotly is here

The other non-obvious deps in this list name the module that imports them (see sbi just above), and plotly's only importer is isaaclab_arena_examples/relations/relation_solver_visualizer.py — nothing in isaaclab_arena itself. Could we add the same one-liner, and is the dev extra (where jupyter already lives) a better home, given the sole consumer is an example notebook?

Suggested change
"plotly",
# Relation solver visualization (isaaclab_arena_examples.relations.relation_solver_visualizer),
# imported at module level.
"plotly",

@arena-review-bot

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

Declares plotly as a direct dependency so isaaclab_arena_examples/relations/relation_solver_visualizer.py stops relying on it arriving transitively. The lock diff is only two lines because plotly was already in uv.lock via isaaclab-dev → open3d → dash → plotly — which is exactly why the native uv env worked while the Docker image (Isaac Lab from the submodule, no open3d) hit the ModuleNotFoundError. Declaring the direct edge is the right fix, and it reaches Docker since the image installs -e "${WORKDIR}/[dev]".

Findings

🔵 Improvement: pyproject.toml:30 — the surrounding deps document who imports them (sbi, moviepy, imageio, pandas); plotly has one importer and it lives in the examples package, so a matching one-line comment would help — and it raises the question of whether the dev extra is the better home. Left inline.

Test Coverage

isaaclab_arena_examples/tests/test_relation_solver_examples.py::test_dummy_object_placer_notebook_runs already imports the plotly-backed visualizer at module level and carries no marker, so it runs in Phase 1 and covers this. No new test needed for a dependency declaration.

Nit (not a code finding)

The last two bullets of the PR description contain a literal \n instead of a line break, so they render as one run-on line.

Verdict

Ship it.

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