Skip to content

Cherry pick: add Plotly runtime dependency (#1212) - #1213

Open
zhx06 wants to merge 1 commit into
release/0.3.0from
zxiao/fix/backport-plotly-dependency
Open

Cherry pick: add Plotly runtime dependency (#1212)#1213
zhx06 wants to merge 1 commit into
release/0.3.0from
zxiao/fix/backport-plotly-dependency

Conversation

@zhx06

@zhx06 zhx06 commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

Cherry-pick the Plotly runtime dependency fix from #1212 onto release/0.3.0.

  • Declare Plotly for the relation solver visualization examples.
  • Regenerate the release lockfile.
  • Verify the Dummy ObjectPlacer example completes successfully in Docker.

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 a note saying why plotly is here

The neighbours in this list explain who imports them (sbi for the sensitivity analysis, pandas for the GR00T HDF5 conversion). plotly's only importer is isaaclab_arena_examples/relations/relation_solver_visualizer.py, which isn't guessable from the name — same one-liner here?

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

@arena-review-bot

arena-review-bot Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

🤖 Isaac Lab-Arena Review Bot

Summary

Backports the plotly runtime dependency onto release/0.3.0 so the relation-solver visualization example stops failing with ModuleNotFoundError. The fix is correct: isaaclab_arena_examples/relations/relation_solver_visualizer.py exists on this branch and imports plotly.graph_objects at module level, and the Docker build installs the root package with its deps (pip install -e "${WORKDIR}/[dev]"), so the image will pick it up. The lockfile regeneration is self-consistent — narwhals and packaging were already present with the exact marker plotly needs, which is why only the one package block was added.

Findings

🔵 Improvement: pyproject.toml:26plotly is inserted directly under the # Sensitivity analysis (isaaclab_arena.analysis.sensitivity) comment, but that module does not use it; the sole importer is isaaclab_arena_examples/relations/relation_solver_visualizer.py. Suggested its own comment inline, to match how sbi/scipy/matplotlib, decorator, and pandas each name their importer.

Cherry-pick ordering

Worth confirming: the source PR #1212 is still open on main and commit b19cf049 is not yet merged there. Backporting ahead of the source means release/0.3.0 silently diverges if #1212 changes during review (e.g. if reviewers ask for the dependency to be pinned or moved behind an extra). Is the intent to land this first deliberately, or should it wait on #1212?

Test Coverage

No test needed — this is a packaging change, and the install itself is the verification. The Detect uv-related changes job is the right gate for lockfile/pyproject consistency; note that the currently-red checks on this SHA are cancelled superseded runs, with fresh Pre-commit and Detect uv-related changes runs still queued, so those should be green before merge.

Verdict

Minor fixes needed

@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Declares Plotly as a runtime dependency for relation-solver visualization examples and updates the release lockfile to resolve Plotly 7.0.0 with its transitive dependencies.

Confidence Score: 5/5

The PR appears safe to merge with no actionable issues identified.

The declared dependency and generated lockfile are consistent, and the locked Plotly release is compatible with the project’s Python constraint and visualization API usage.

Important Files Changed

Filename Overview
pyproject.toml Adds Plotly to the project’s runtime dependencies.
uv.lock Records the consistent Plotly 7.0.0 resolution and transitive dependency metadata.

Reviews (2): Last reviewed commit: "Add Plotly runtime dependency" | Re-trigger Greptile

@zhx06
zhx06 force-pushed the zxiao/fix/backport-plotly-dependency branch from 614d823 to ec14cb4 Compare September 9, 2026 20:51
@zhx06 zhx06 closed this Sep 9, 2026
@zhx06
zhx06 deleted the zxiao/fix/backport-plotly-dependency branch September 9, 2026 20:53
@zhx06
zhx06 restored the zxiao/fix/backport-plotly-dependency branch September 9, 2026 20:53
@zhx06 zhx06 reopened this Sep 9, 2026
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.

🔵 This lands under a comment that names the wrong module

plotly sits directly beneath the "Sensitivity analysis" comment, but nothing in isaaclab_arena.analysis.sensitivity imports it — the only importer in the repo is isaaclab_arena_examples/relations/relation_solver_visualizer.py. Every other non-obvious dep in this list says who pulls it in, so could we give plotly its own note and keep the grouping accurate?

Suggested change
"plotly",
# 3D relation-solver visualization
# (isaaclab_arena_examples.relations.relation_solver_visualizer), imported at module level.
"plotly",

Signed-off-by: zhx06 <zihaox@nvidia.com>
(cherry picked from commit b19cf04)
@zhx06
zhx06 force-pushed the zxiao/fix/backport-plotly-dependency branch from ec14cb4 to e34f296 Compare September 9, 2026 23:39
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.

2 participants