feat(newton): stub NewtonSimulation(SimEngine) + config + factory registration#102
Closed
cagataycali wants to merge 37 commits intostrands-labs:mainfrom
Closed
feat(newton): stub NewtonSimulation(SimEngine) + config + factory registration#102cagataycali wants to merge 37 commits intostrands-labs:mainfrom
cagataycali wants to merge 37 commits intostrands-labs:mainfrom
Conversation
Changed logger.debug to logger.warning when auto-download module is unavailable, so users see actionable guidance to install robot_descriptions.
…C21) Added logging import and changed bare 'except Exception: pass' to log the error at debug level. This helps diagnose cleanup bugs while still preventing GC exceptions from propagating.
Changed load_scene, run_policy, randomize, get_contacts from returning error dicts to raising NotImplementedError. This makes unimplemented features explicit during development. Updated tests accordingly.
Added logger.debug for _HAS_ASSET_MANAGER so users can diagnose why model resolution may fail.
…C28) Replaced inline lazy import of strands_robots.registry inside resolve_urdf() with top-level try/except and _HAS_REGISTRY guard. Cleaner and follows project conventions.
… C30) - register_backend now raises ValueError if name/alias already exists unless force=True is passed (C22, C23) - Protects built-in aliases (mj, mjc, mjx) from accidental override - Fixed test to use lambda: FakeBackend (correct loader signature) and verify create_simulation returns correct instance (C30) - Added tests for duplicate rejection and alias conflict detection
Renamed primary ABC class from SimulationBackend to SimEngine for consistency with Sim* prefix convention (SimWorld, SimRobot, SimObject). SimulationBackend kept as backward-compatible alias. Updated factory, __init__, and all tests.
- Removed mujoco/ subtree from Architecture docstring since those modules don't exist in this PR (they come in subsequent PRs) - Cleared _LAZY_IMPORTS of mujoco-specific entries that would cause ModuleNotFoundError - Commented out Simulation/MuJoCoSimulation/MJCFBuilder from __all__ - Simplified Usage docstring to show only what's available now
…s (C27) Changed resolve_model() to check local/custom paths first, then fall back to Menagerie asset manager. This ensures users who place custom URDFs in ./urdfs/ or STRANDS_URDF_DIR get their version, not the default Menagerie one.
… C8, C10) - assets/__init__.py reduced from 282 to 42 lines (thin exports only) - All implementation moved to assets/manager.py - Fixed docstring: clarified assets are NOT bundled, meshes are downloaded on-demand from robot_descriptions/GitHub (C4) - Added comment explaining lazy import of download_assets (C8) - download.py now re-exports from tools/download_assets.py (C10) This addresses the core structural concern about __init__.py being too heavy (AGENTS.md Convention strands-labs#3).
…S_URDF_DIR (C5) STRANDS_ASSETS_DIR is now the single canonical env var for custom asset paths. STRANDS_URDF_DIR still works but emits a DeprecationWarning guiding users to switch.
- models.py: SimWorld, SimRobot, SimObject, SimCamera, TrajectoryStep, SimStatus dataclasses for typed simulation state management (C24) - tools/download_assets.py: robot asset download via robot_descriptions with git clone fallback. Note: download logic is the single source of truth, consumed by assets/manager.py (addresses C11 concern)
Reduced search paths from 5 to 2 (plus env var overrides): - ~/.strands_robots/assets/ (user cache) - CWD/assets/ (project-local) Added documentation explaining the resolution order and pointing to resolve_model() as the preferred API.
…er docstring (C17, C20) - Added Method categories section to SimEngine docstring explaining the rationale for required (@AbstractMethod) vs optional methods - Improved render() docstring to specify return type: dict with 'image' (numpy RGB uint8) and optional 'depth' (float32)
…E (C6, C7) Documented STRANDS_ASSETS_DIR, GROOT_API_TOKEN env vars in a table. Added cache directory section explaining ~/.strands_robots/assets/ purpose, how to clear it, and how to override location.
…acade methods (C16, C18) Added docstrings explaining these are convenience/facade methods that delegate to Robot abstraction. SimEngine intentionally provides a unified interface for agent tools (simulation tool actions) without requiring users to understand the Robot/Policy/Sim separation.
…levels Mypy fixes (25 errors → 0): - Add from __future__ import annotations to all simulation modules - Fix implicit Optional params (float = None → float | None = None) - Add missing return type annotations (__post_init__ → None, etc.) - Add **kwargs: Any type annotations to abstract methods - Fix Any return types with explicit str() casts Review feedback: - Remove SimulationBackend backward-compat alias from base.py — not needed since this PR introduces SimEngine (self-review thread strands-labs#35) - Remove SimulationBackend from __init__.py exports and __all__ - Remove stale comments from __init__.py (thread strands-labs#36) - Fix __del__ log level: logger.debug → logger.warning (thread strands-labs#26) - Fix asset manager log level: logger.debug → logger.info (thread strands-labs#31) - Update test: remove test_backward_compat_alias (alias removed) Tests: 24/24 pass, ruff clean, mypy clean
…ts.py - assets/manager.py: 4 no-any-return errors — add str() casts for dict subscript returns used in Path operations (dict[str,Any]['key'] → Any) - tools/download_assets.py: annotate _needs_download/_get_source params as dict[str,Any], add type annotations to local variables, fix PEP 484 implicit Optional (robots: str = None → str | None = None), filter None from all_sim dict comprehension
…riptions to stubs 1. download_assets.py:41 — type: ignore[misc] → type: ignore[no-redef] (mypy error code was no-redef, not misc) 2. pyproject.toml — add robot_descriptions.* to ignore_missing_imports (optional dep without type stubs, same as lerobot/torch/etc) Tests: 290 passed, mypy clean, ruff clean.
…fix mypy Registry: - Expand robots.json from 38 to 68 robots (all MuJoCo-validated) - Add new categories: aerial, expressive, mobile_manip - New robots include Stretch, Spot, Sawyer, ANYmal, Crazyflie, etc. - Sort entries alphabetically by category then name User registration API (register_robot/unregister_robot): - New strands_robots/registry/user_registry.py - Persists to ~/.strands_robots/user_robots.json - Merged at load time via loader._merge_user_robots() - Supports aliases, hardware config, robot_descriptions_module - Validates model_xml exists, normalizes names - Exported from strands_robots.registry.__init__ Shared path utilities (strands_robots/utils.py): - get_base_dir(), get_assets_dir(), resolve_asset_path() - Single source of truth for STRANDS_ASSETS_DIR resolution - Used by user_registry, model_registry, asset manager mypy: 37 → 0 errors: - simulation/base.py: explicit Optional types, return annotations - simulation/factory.py: rename cls → backend_cls (no-redef) - simulation/models.py: __post_init__ return type - simulation/model_registry.py: register_urdf return type, str() wraps - tools/download_assets.py: Optional params, None guards, type annotations - assets/manager.py: typed candidates list, Path() wraps Tests: - New tests/test_user_registry.py (364 lines, 20 test cases) - 324 tests pass, ruff clean, mypy 0 errors
The code in assets/manager.py and tools/download_assets.py references robot_descriptions for automatic asset downloads, and the docstrings reference 'pip install strands-robots[sim]', but the [sim] extra was never declared in pyproject.toml. Add: - sim extra: robot_descriptions>=1.11.0,<2.0.0 - Include [sim] in [all] extra
…ndomize base Expand the base SimEngine.add_object signature with orientation and mesh_path parameters needed by concrete backends (MuJoCo). Simplify randomize() docstring — backends define their own params.
1. manager.py: Fix docstring — we don't bundle assets, use robot_descriptions 2. manager.py: Remove _BUNDLED_DIR and redundant STRANDS_ASSETS_DIR handling 3. model_registry.py: Remove 'legacy' prefix, remove redundant 'as' aliases 4. download_assets.py: Simplify strands import (strands is a required dep) 5. download_assets.py: Remove CLI main() — tool is the entry point 6. download_assets.py: Document :3 mesh check heuristic 7. robots.json: Remove empty aliases arrays (omit key instead) 8. user_registry.py: Use public invalidate_cache() instead of private _cache/_mtimes 9. models.py: Replace MuJoCo-specific field comments with generic engine comments Tests: 84 passed, 0 failures (0.89s)
- Add invalidate_cache to __all__ in registry/__init__.py - Merge unsorted imports in simulation/model_registry.py - Remove unused argparse import in tools/download_assets.py - Fix import block formatting in tools/download_assets.py
main() was removed from tools/download_assets.py in 2cfcbfc but the import in assets/download.py was not cleaned up, breaking mypy: strands_robots/assets/download.py:7: error: Module "strands_robots.tools.download_assets" has no attribute "main" Removes: main import, __all__ entry, __main__ block. Tests: 238 passed, 6 skipped (2.29s)
Per @awsarron review (PR strands-labs#84 threads on download.py and download_assets.py): tools should be lightweight wrappers that expose strands-robots library functions to agents, not contain the actual implementation. Before: tools/download_assets.py (493 lines) — all download logic + @tool assets/download.py (14 lines) — just re-exports from tools After: assets/download.py (421 lines) — all download logic (library code) tools/download_assets.py (78 lines) — thin @tool wrapper Changes: - assets/download.py: moved download_robots(), all backends (_download_via_robot_descriptions, _download_via_git, _download_from_github), and helpers from tools/download_assets.py - tools/download_assets.py: slim @tool wrapper that parses action/args and delegates to assets.download.download_robots() - assets/manager.py: fix _auto_download_robot() lazy import from tools.download_assets → .download (same package, relative import) - tools/__init__.py: add download_assets to lazy imports No circular imports: assets/download.py imports from assets/manager.py at module level; manager.py lazy-imports from assets/download.py inside _auto_download_robot() only. Tests: 238 passed, 6 skipped, 0 failures.
Per @awsarron review thread — removed tests that test Python itself (dataclass defaults, dict membership, enum values) rather than behavior. Removed: - test_sim_robot_defaults — tests dataclass default values - test_sim_robot_custom_position — tests kwarg passthrough - test_sim_object_defaults — tests dataclass defaults - test_sim_camera_defaults — tests dataclass defaults - test_sim_world_defaults — tests dataclass defaults - test_sim_status_enum — tests enum values (Python itself) - test_trajectory_step_default_instruction — tests default empty string - test_list_backends_returns_list — tests isinstance(list) - test_resolve_known_model — tests None-or-string (weak) - test_resolve_unknown_returns_none — tests None return Kept all behavioral tests: factory round-trip, context manager cleanup, registration validation, URDF registration+resolution, model table output. Consolidated remaining dataclass tests into TestSimModelsUsage with behavioral names (tracks_robots, preserves_originals, records_episode_data). Tests: 228 passed, 6 skipped, 0 failures (was 238).
…s, legacy docs, mesh check Addresses review comments from @awsarron on PR strands-labs#84: - Fix ruff I001 import sort in test_simulation_foundation.py (CI fix) - manager.py: remove redundant STRANDS_ASSETS_DIR handling — get_assets_dir() is now the single source of truth (lines 46-52 were redundant) - models.py: remove MuJoCo-specific references from base dataclasses — now says 'engine-specific handle (set by simulation backend)' instead of referencing mj.MjModel/mj.MjData - model_registry.py: document 'legacy_urdf' meaning (backward-compat URDF paths from before MJCF asset system), remove redundant 'as' aliases in import block - download.py: remove ':3' mesh check limit — now validates all mesh files instead of sampling first 3 - download_assets.py: remove unused main(), add README docstring about auto-download sources (robot_descriptions, git, HuggingFace)
_resolve_robot_descriptions_module returns dict .get() result which mypy sees as Any. Add type annotation and str() cast to satisfy warn_return_any. Fixes CI failure on ca4ca6c.
Move _xml, _robot_base_xml, and _tmpdir from SimWorld into a generic _backend_state dict. Each backend stores its format-specific data there instead of polluting the base class with implementation details. Addresses @awsarron review: 'how can we avoid having implementation details (Mujoco) in base classes like this?' The MuJoCo backend (PR strands-labs#85) will store these in world._backend_state['xml'], etc. during rebase.
…on in _shallow_clone Address @mrgh-test review comments on PR strands-labs#84: 1. manager.py: Add _safe_join() validation when resolving asset paths. Registry-sourced asset_dir_name and xml_file values are now validated against path-traversal attacks (../) before filesystem access. Extracted _resolve_candidates() helper to centralise the safe-join + search loop that was repeated 3 times in resolve_model_path(). Also applied _safe_join in resolve_model_dir(). 2. download.py: Move URL validation into _shallow_clone() itself. Added _ALLOWED_CLONE_URL_RE that only accepts HTTPS github.com URLs. This prevents ssh://, git://, file:// and other schemes regardless of the caller, removing the burden from individual call sites. Updated exception handlers to catch ValueError from the new validation. Tests: 228 passed, 6 skipped, 0 failures. Lint: ruff + mypy clean.
Address @mrgh-test callout on PR strands-labs#84 thread 3 (user_registry.py:95): register_robot() must not be exposed as an agent @tool without STRANDS_TRUST_REMOTE_CODE gating + path validation, since malicious MJCF could execute code via MuJoCo plugins. Tests: 228 passed, 6 skipped, 0 failures.
…factory registration
Add the Newton GPU-accelerated simulation backend skeleton:
- NewtonSimulation(SimEngine) stub class with all 12 abstract methods
raising NotImplementedError until follow-up PRs land the real logic
- NewtonConfig dataclass with validation for solver, render_backend,
broad_phase, physics_dt, and num_envs
- SOLVER_MAP with 7 backends (mujoco, featherstone, semi_implicit,
xpbd, vbd, style3d, implicit_mpm) validated on Jetson AGX Thor
- Factory registration: 'newton' in _BUILTIN_BACKENDS, 'warp' alias
- Lazy imports: importing the newton package does NOT trigger warp load
- create_simulation('newton', num_envs=4096) works end-to-end
- 39 tests covering factory, class hierarchy, config validation,
solver constants, and lazy import guards
- 100% coverage on all new files
Part 1 of 7 for issue cagataycali/strands-gtc-nvidia#314.
Depends on: strands-labs#84 (SimEngine ABC)
cagataycali
force-pushed
the
feat/newton-stub
branch
from
db4f57e to
087e445
Compare
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
2 participants
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
Add Newton GPU simulation backend stub implementing the
SimEngineABC. This is PR 1 of 7 in the Newton backend port (cagataycali/strands-gtc-nvidia#314).What this PR does
strands_robots/simulation/newton/__init__.py__getattr__— importing does NOT trigger Warp/CUDA initstrands_robots/simulation/newton/config.pyNewtonConfigdataclass — validates 7 solvers, render backends, broad-phase. Pure stdlibstrands_robots/simulation/newton/simulation.pyNewtonSimulation(SimEngine)— all 12 abstract methods stubNotImplementedErrorstrands_robots/simulation/newton/solvers.pySOLVER_MAP,RENDER_BACKENDS,BROAD_PHASE_OPTIONSconstantsstrands_robots/simulation/factory.py"newton"backend +"warp"aliastests/simulation/newton/test_newton_stub.pyWhat this enables
Test results
Changes to factory.py (minimal)
"newton"in_BUILTIN_BACKENDS→ lazy-loadsNewtonSimulation"warp": "newton"to_BUILTIN_ALIASESDesign decisions
SimEnginesubclass — noAgentToolmixin (thin wrapper can be added later)__init__.py—__getattr__with globals caching, matches simulation package patterncreate_simulation("newton", solver="xpbd")auto-constructsNewtonConfigValueErrorat construction, not atstep()PR chain (7 PRs total)
🤖 AI agent response. Strands Agents. Feedback welcome!