Add Newton course notebook, improve ViewerViser#2415
Draft
eric-heiden wants to merge 15 commits intonewton-physics:mainfrom
Draft
Add Newton course notebook, improve ViewerViser#2415eric-heiden wants to merge 15 commits intonewton-physics:mainfrom
eric-heiden wants to merge 15 commits intonewton-physics:mainfrom
Conversation
## Motivation
Per project guidelines in `AGENTS.md`, user-facing additions must be recorded in the `[Unreleased]` section of `CHANGELOG.md`. This PR adds a substantial new tutorial notebook (`docs/tutorials/01_newton_course.ipynb`) but had no corresponding changelog entry.
## What changed
- Appended one line to `### Added` in the `[Unreleased]` section:
`- Add introductory tutorial notebook covering ModelBuilder, solvers, CUDA graphs, IK, and pick-and-place`
## Why this fix is correct
- Uses imperative present tense ("Add …") per commit/changelog conventions.
- Placed at the end of `### Added`, keeping the existing order intact.
- Covers all major topics in the notebook (ModelBuilder, solvers, CUDA graphs, IK, pick-and-place) without leaking internal implementation details.
## Verification
- `grep -n "tutorial" CHANGELOG.md` confirms the new entry at line 28.
- Visual inspection of `git diff` confirms a single clean insertion with no unintended whitespace or formatting changes.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Contributor
📝 WalkthroughWalkthroughAdds a large Newton tutorial notebook and changelog entries, refactors ViewerViser to construct Jupyter-proxied playback URLs and locate installed Viser client assets, renders plane instances as line-grid meshes, pins Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant JupyterProxy as "Jupyter Server Proxy"
participant LocalServer as "Viewer Local Server (127.0.0.1:{port})"
participant ViserClient as "Viser Client Assets (installed package)"
Browser->>JupyterProxy: Request proxied URL /proxy/{port}/?playbackPath=recording.viser
JupyterProxy->>LocalServer: Forward HTTP request to 127.0.0.1:{port}
LocalServer->>ViserClient: Serve player HTML referencing client assets (from installed viser)
ViserClient-->>LocalServer: Provide assets (index.html, JS, CSS)
LocalServer-->>JupyterProxy: Return assembled recording page
JupyterProxy-->>Browser: Return proxied response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/tutorials/01_newton_course.ipynb`:
- Around line 433-440: In simulate() the code currently creates a new contacts
buffer each substep via contacts = model.collide(state_0); change this to reuse
a preallocated contacts buffer by calling model.collide(state_0, contacts) so
collide populates the existing buffer in-place (matching the pattern used
elsewhere in the notebook); locate the contacts variable referenced in
simulate() and pass it as the second argument to model.collide rather than
assigning the return value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 25eba3c4-a12e-4f42-b026-0f95f7822232
⛔ Files ignored due to path filters (2)
docs/tutorials/images/break.jpgis excluded by!**/*.jpgdocs/tutorials/images/cuda-graph.pngis excluded by!**/*.png
📒 Files selected for processing (2)
CHANGELOG.mddocs/tutorials/01_newton_course.ipynb
Comment on lines
+433
to
+440
| "def simulate():\n", | ||
| " \"\"\"Run one frame of simulation (multiple substeps).\"\"\"\n", | ||
| " global state_0, state_1\n", | ||
| "\n", | ||
| " for _ in range(sim_substeps):\n", | ||
| " state_0.clear_forces()\n", | ||
| " contacts = model.collide(state_0)\n", | ||
| " solver.step(state_in=state_0, state_out=state_1, control=control, contacts=contacts, dt=sim_dt)\n", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Model.collide signature/behavior around optional contacts argument.
fd -i "model.py" -t f | xargs -r rg -n -C4 'def collide\(|if contacts is None|self\.contacts\('
# Show collide call sites in this notebook for consistency checks.
rg -n -C2 'model\.collide\(' docs/tutorials/01_newton_course.ipynbRepository: newton-physics/newton
Length of output: 2705
Fix the suboptimal contacts allocation pattern for consistency with the rest of the tutorial.
The code at line 439 allocates a fresh contacts buffer each substep (contacts = model.collide(state_0)), but the Model.collide() method accepts an optional contacts parameter for in-place population. Later in the notebook (lines 688, 778, 1499), the pattern correctly reuses a preallocated buffer (model.collide(state_0, contacts)). Update line 439 to match this efficient, consistent pattern.
♻️ Proposed change
def simulate():
"""Run one frame of simulation (multiple substeps)."""
global state_0, state_1
for _ in range(sim_substeps):
state_0.clear_forces()
- contacts = model.collide(state_0)
+ model.collide(state_0, contacts)
solver.step(state_in=state_0, state_out=state_1, control=control, contacts=contacts, dt=sim_dt)
# Swap buffers so state_0 always holds the latest state
state_0, state_1 = state_1, state_0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tutorials/01_newton_course.ipynb` around lines 433 - 440, In simulate()
the code currently creates a new contacts buffer each substep via contacts =
model.collide(state_0); change this to reuse a preallocated contacts buffer by
calling model.collide(state_0, contacts) so collide populates the existing
buffer in-place (matching the pattern used elsewhere in the notebook); locate
the contacts variable referenced in simulate() and pass it as the second
argument to model.collide rather than assigning the return value.
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Load notebook playback assets directly from the installed `viser` package instead of shipping a separate Newton copy so the browser client stays compatible with the serializer version in the active environment. Also update the pinned `viser` dependency, adjust docs asset copying, add regression coverage, and fix tutorial notebook lint issues so pre-commit passes for the branch.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/tutorials/01_newton_course.ipynb (1)
433-436:⚠️ Potential issue | 🟡 MinorReuse the preallocated
contactsbuffer in this loop.This section already created
contacts = model.contacts(), but Line 435 replaces it with a fresh buffer on every substep. Usingmodel.collide(state_0, contacts)keeps the tutorial consistent with the later sections and avoids needless allocations before the CUDA-captured variant.♻️ Proposed fix
for _ in range(sim_substeps): state_0.clear_forces() - contacts = model.collide(state_0) + model.collide(state_0, contacts) solver.step(state_in=state_0, state_out=state_1, control=control, contacts=contacts, dt=sim_dt) # Swap buffers so state_0 always holds the latest state state_0, state_1 = state_1, state_0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/tutorials/01_newton_course.ipynb` around lines 433 - 436, The loop currently reassigns a fresh contacts buffer each substep; instead reuse the preallocated buffer created earlier (contacts = model.contacts()) by calling model.collide(state_0, contacts) inside the for loop so the existing contacts object is filled rather than replaced; update the call in the loop where solver.step(...) is invoked to pass the reused contacts buffer (use model.collide(state_0, contacts) before solver.step) to avoid repeated allocations and match later CUDA-captured usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/conf.py`:
- Around line 406-413: The docs conf currently only adds the installed viser
package's "client/build" path to src_candidates; update the block that imports
inspect and viser to also append the alternative "static" fallback used by
newton/_src/viewer/viewer_viser.py (i.e., add
Path(inspect.getfile(viser)).resolve().parent / "static") so Sphinx will copy
assets when the package exposes the fallback layout; keep both additions using
the same Path(inspect.getfile(viser)).resolve().parent base and append to
src_candidates like the existing "client/build" entry.
In `@newton/_src/viewer/viewer_viser.py`:
- Around line 192-194: The startup log in __init__ prints a hardcoded
"http://localhost:{self._port}" instead of the proxy-aware address; change the
verbose startup/message to use self.url (which already resolves via
is_jupyter_notebook and _build_browser_url using _port) so the log shows the
proxied "/proxy/<port>/..." path in notebook environments; update the log call
in __init__ to reference self.url rather than constructing localhost, and ensure
self.url is computed before the log is emitted.
---
Duplicate comments:
In `@docs/tutorials/01_newton_course.ipynb`:
- Around line 433-436: The loop currently reassigns a fresh contacts buffer each
substep; instead reuse the preallocated buffer created earlier (contacts =
model.contacts()) by calling model.collide(state_0, contacts) inside the for
loop so the existing contacts object is filled rather than replaced; update the
call in the loop where solver.step(...) is invoked to pass the reused contacts
buffer (use model.collide(state_0, contacts) before solver.step) to avoid
repeated allocations and match later CUDA-captured usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 44209f5f-7081-4d90-b5eb-7ef352fdec04
⛔ Files ignored due to path filters (4)
newton/_src/viewer/viser/static/Inter-VariableFont_slnt,wght.ttfis excluded by!**/*.ttfnewton/_src/viewer/viser/static/assets/Sorter-Df0J3ZWJ.wasmis excluded by!**/*.wasmnewton/_src/viewer/viser/static/logo.svgis excluded by!**/*.svguv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CHANGELOG.mddocs/conf.pydocs/tutorials/01_newton_course.ipynbnewton/_src/viewer/viewer_viser.pynewton/_src/viewer/viser/static/assets/SplatSortWorker-DiSpcAPr.jsnewton/_src/viewer/viser/static/assets/WebsocketServerWorker-C6PJJ7Dx.jsnewton/_src/viewer/viser/static/assets/__vite-browser-external-BIHI7g3E.jsnewton/_src/viewer/viser/static/assets/index-BVvA0mmR.cssnewton/_src/viewer/viser/static/assets/index-H4DT9vxj.jsnewton/_src/viewer/viser/static/index.htmlnewton/_src/viewer/viser/static/manifest.jsonnewton/_src/viewer/viser/static/robots.txtpyproject.toml
💤 Files with no reviewable changes (6)
- newton/_src/viewer/viser/static/assets/__vite-browser-external-BIHI7g3E.js
- newton/_src/viewer/viser/static/manifest.json
- newton/_src/viewer/viser/static/index.html
- newton/_src/viewer/viser/static/robots.txt
- newton/_src/viewer/viser/static/assets/SplatSortWorker-DiSpcAPr.js
- newton/_src/viewer/viser/static/assets/WebsocketServerWorker-C6PJJ7Dx.js
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
Comment on lines
+406
to
+413
| # Installed viser package build. Prefer this so the copied docs assets stay in | ||
| # sync with the serializer version used for notebook playback. | ||
| try: | ||
| import newton # noqa: PLC0415 | ||
| import inspect # noqa: PLC0415 | ||
|
|
||
| import viser # noqa: PLC0415 | ||
|
|
||
| src_candidates.append(Path(newton.__file__).resolve().parent / "_src" / "viewer" / "viser" / "static") | ||
| src_candidates.append(Path(inspect.getfile(viser)).resolve().parent / "client" / "build") |
Contributor
There was a problem hiding this comment.
Add the same static fallback used by ViewerViser.
newton/_src/viewer/viewer_viser.py checks both client/build and static, but this docs-side copy path only registers client/build. If the installed viser package exposes the fallback layout, Sphinx will quietly skip copying the player assets and the embedded notebook replays will end up with a broken ../_static/viser/index.html.
♻️ Proposed fix
try:
import inspect # noqa: PLC0415
import viser # noqa: PLC0415
- src_candidates.append(Path(inspect.getfile(viser)).resolve().parent / "client" / "build")
+ viser_package_dir = Path(inspect.getfile(viser)).resolve().parent
+ src_candidates.extend(
+ [
+ viser_package_dir / "client" / "build",
+ viser_package_dir / "static",
+ ]
+ )
except Exception:
pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/conf.py` around lines 406 - 413, The docs conf currently only adds the
installed viser package's "client/build" path to src_candidates; update the
block that imports inspect and viser to also append the alternative "static"
fallback used by newton/_src/viewer/viewer_viser.py (i.e., add
Path(inspect.getfile(viser)).resolve().parent / "static") so Sphinx will copy
assets when the package exposes the fallback layout; keep both additions using
the same Path(inspect.getfile(viser)).resolve().parent base and append to
src_candidates like the existing "client/build" entry.
Comment on lines
+192
to
194
| if self.is_jupyter_notebook: | ||
| return self._build_browser_url(self._port) | ||
| return f"http://localhost:{self._port}" |
Contributor
There was a problem hiding this comment.
Make the startup log use the proxy-aware URL too.
url can now resolve to a proxied /proxy/<port>/... path in notebooks, but __init__ still prints http://localhost:{port} unconditionally. In remote Jupyter sessions that message is misleading and often unusable, so the verbose log should print self.url instead.
♻️ Suggested follow-up
- if verbose:
- print(f"Viser server running at: http://localhost:{port}")
+ if verbose:
+ print(f"Viser server running at: {self.url}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@newton/_src/viewer/viewer_viser.py` around lines 192 - 194, The startup log
in __init__ prints a hardcoded "http://localhost:{self._port}" instead of the
proxy-aware address; change the verbose startup/message to use self.url (which
already resolves via is_jupyter_notebook and _build_browser_url using _port) so
the log shows the proxied "/proxy/<port>/..." path in notebook environments;
update the log call in __init__ to reference self.url rather than constructing
localhost, and ensure self.url is computed before the log is emitted.
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
eric-heiden
changed the title
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 (2)
newton/_src/viewer/viewer_viser.py (2)
1387-1389:⚠️ Potential issue | 🟠 MajorBind playback server to loopback only.
The playback URL is built for
127.0.0.1, but the HTTP server binds to all interfaces. This unnecessarily exposes notebook playback assets on the network.♻️ Proposed fix
- # Bind to all interfaces so IFrame can access it - server = HTTPServer(("", port), handler_class) + # Bind to loopback; notebook iframes/proxies can still access this. + server = HTTPServer(("127.0.0.1", port), handler_class)Also applies to: 1397-1401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/viewer/viewer_viser.py` around lines 1387 - 1389, The HTTPServer is currently bound to all interfaces via HTTPServer(("", port), handler_class) which exposes playback assets externally; change the bind address to loopback only (e.g., use "127.0.0.1" or "localhost" instead of "" ) in both places where HTTPServer is instantiated (the lines constructing server = HTTPServer(...)) so the playback server only listens on the local interface; ensure any URL generation still matches the chosen loopback host.
188-208:⚠️ Potential issue | 🟠 MajorClear plane-grid state in
clear_model()to avoid stale overlays.
_plane_handlesand_plane_meshesare not reset inclear_model(). Since plane grids are tracked outside_scene_handles, they can survive model resets.♻️ Proposed fix
def clear_model(self): """Reset model-dependent state, including scalar plot buffers.""" + for handles in self._plane_handles.values(): + for handle in handles: + try: + handle.remove() + except Exception: + pass + self._plane_handles.clear() + self._plane_meshes.clear() + # Remove plot handles from the GUI. for handle in self._plot_handles.values(): try: handle.remove() except Exception:Also applies to: 595-603, 664-665
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/viewer/viewer_viser.py` around lines 188 - 208, clear_model currently resets plot-related state but leaves plane overlays alive; update viewer_viser.clear_model to also remove and clear _plane_handles and _plane_meshes: iterate over self._plane_handles and call handle.remove() in a try/except then clear the dict/list, and if plane mesh objects exist call their remove() or set them to None and clear self._plane_meshes. Apply the same cleanup pattern used for _plot_handles/_plot_folder/_scalar_* to the plane state so plane grids cannot persist across model resets (also replicate this change wherever other model-reset helpers exist that manage plane state).
♻️ Duplicate comments (1)
newton/_src/viewer/viewer_viser.py (1)
167-174:⚠️ Potential issue | 🟡 MinorUse proxy-aware URL in startup log (still prints localhost).
The verbose message is still hardcoded to localhost, which is misleading in notebook-proxy environments. Please log
self.urlafter_portandis_jupyter_notebookare initialized.♻️ Proposed fix
- if verbose: - print(f"Viser server running at: http://localhost:{port}") - - # Store configuration - self._port = port - - # Track if running in Jupyter - self.is_jupyter_notebook = is_jupyter_notebook() + # Store configuration + self._port = port + + # Track if running in Jupyter + self.is_jupyter_notebook = is_jupyter_notebook() + + if verbose: + print(f"Viser server running at: {self.url}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/viewer/viewer_viser.py` around lines 167 - 174, The startup verbose log currently prints a hardcoded localhost URL before instance state is set; move the verbose print to after assigning self._port and computing self.is_jupyter_notebook, and change it to log the proxy-aware URL by using self.url (instead of hardcoded "http://localhost:{port}") so notebook-proxy environments show the correct address; update the code around the assignments to self._port and is_jupyter_notebook = is_jupyter_notebook() and print self.url when verbose is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@newton/_src/viewer/viewer_viser.py`:
- Around line 1387-1389: The HTTPServer is currently bound to all interfaces via
HTTPServer(("", port), handler_class) which exposes playback assets externally;
change the bind address to loopback only (e.g., use "127.0.0.1" or "localhost"
instead of "" ) in both places where HTTPServer is instantiated (the lines
constructing server = HTTPServer(...)) so the playback server only listens on
the local interface; ensure any URL generation still matches the chosen loopback
host.
- Around line 188-208: clear_model currently resets plot-related state but
leaves plane overlays alive; update viewer_viser.clear_model to also remove and
clear _plane_handles and _plane_meshes: iterate over self._plane_handles and
call handle.remove() in a try/except then clear the dict/list, and if plane mesh
objects exist call their remove() or set them to None and clear
self._plane_meshes. Apply the same cleanup pattern used for
_plot_handles/_plot_folder/_scalar_* to the plane state so plane grids cannot
persist across model resets (also replicate this change wherever other
model-reset helpers exist that manage plane state).
---
Duplicate comments:
In `@newton/_src/viewer/viewer_viser.py`:
- Around line 167-174: The startup verbose log currently prints a hardcoded
localhost URL before instance state is set; move the verbose print to after
assigning self._port and computing self.is_jupyter_notebook, and change it to
log the proxy-aware URL by using self.url (instead of hardcoded
"http://localhost:{port}") so notebook-proxy environments show the correct
address; update the code around the assignments to self._port and
is_jupyter_notebook = is_jupyter_notebook() and print self.url when verbose is
true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2e8f2518-bdd6-4846-bb3a-f5f9895f9c5e
📒 Files selected for processing (3)
CHANGELOG.mddocs/tutorials/01_newton_course.ipynbnewton/_src/viewer/viewer_viser.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
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.
This PR contributes the course notebook that has been adapted from the GTC hands-on lab, see recording at https://www.nvidia.com/en-us/on-demand/session/gtc26-dlit81700/.
This is not ready for merging yet - we should evaluate the tutorial notebooks in a separate CI job, not the docs build job that runs on every PR on CPU. I will add a separate runner for it in this PR later.
ViewerViser improvements
index.htmlfor playback of Viser recordings in iFrames (e.g. Jupyter notebook cells) that is bundled with the viser dependency instead of shipping our own static HTML/WASM/CSS files (removed them here)Now we have:
Summary by CodeRabbit
Documentation
Changelog
New Features
Removals
Chores