Enable SimReady search API in agentic env gen - #982
Conversation
Greptile SummaryAdds opt-in SimReady asset discovery to agentic environment generation.
Confidence Score: 4/5The packaging incompatibility must be fixed before merging because no supported runtime can currently use the advertised SimReady search feature. Native uv rejects the new extra alongside either IsaacLab group due to the declared dependency conflicts, and the Docker image does not install the extra, so enabled searches return no candidates in every supported installation path. Files Needing Attention: pyproject.toml and docker/Dockerfile.isaaclab_arena Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User prompt] --> B[Spec inference]
B --> C{Catalog asset?}
C -->|Yes| D[Validated Arena asset]
C -->|No| E[SimReady search]
E --> F{One rigid body?}
F -->|Yes| G[Attach USD path and Physics variant]
F -->|No| H[Name unavailable object]
H --> I[One replacement inference pass]
I --> B
D --> J[Generated environment spec]
G --> J
Reviews (1): Last reviewed commit: "fix test and uv test" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| [ | ||
| { extra = "simready" }, | ||
| { group = "isaaclab-from-wheel" }, | ||
| ], | ||
| [ | ||
| { extra = "simready" }, | ||
| { group = "isaaclab-from-source" }, | ||
| ], |
There was a problem hiding this comment.
When a user enables SimReady search, native uv rejects the extra alongside either supported IsaacLab group, while the Docker image installs only [dev] and therefore reaches the missing-package fallback. As a result, neither supported runtime can perform the advertised search.
Knowledge Base Used: Build, Test, and Docker Infrastructure
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
🤖 Isaac Lab-Arena Review BotSummaryThis PR adds an opt-in SimReady asset search to agentic environment generation: objects the Arena catalog can't cover are left to SimReady, searched by phrase after spec inference, filtered to whole-word matches on the object noun, and accepted only if the USD holds exactly one rigid body. It's cleanly off-by-default (CLI flag + GUI expander), keeps SimReady names/URLs out of the asset classes to avoid a pre-sim Findings🟡 🔵 Test CoverageStrong. New unit tests cover word-splitting/ranking, rigid-body rejection, the search fall-through, the retry/unavailable-object flow in the agent, the registered VerdictShip it (minor cleanups optional). |
qianl-nv
left a comment
qianl-nv
left a comment
There was a problem hiding this comment.
Thanks for jumping on this!
have one large size comment about how the inference calls are structured. happy to discuss in more detail.
did a quick pass on the rest. will do a more detailed review after we resolve the large refactor comment.
| task: | ||
| composition: parallel | ||
| description: Pick up the pepsi can and tuna can from the maple table and place them | ||
| into the mini plastic basket, place the bean can next to the basket and the hammer |
There was a problem hiding this comment.
the second part of the description is irrelevant, remove
There was a problem hiding this comment.
interesting how it was picked up by the agent tho...
| if spec is None: | ||
| return None, data | ||
| attempts_left = SIMREADY_RETRY_ATTEMPTS | ||
| while True: |
There was a problem hiding this comment.
It feels like we are overloading the spec_inference call.
I'd prefer something that's a middle ground to between this and the full prompt normalization; have one optional inference call that only runs when sim-ready is enabled. It's given the prompt, the existing asset catalogue, and as to return a list of search phrase for any potentially missing assets.
This way we avoids change to existing interference path without sim-ready, and avoids this kinda hard to read loop that overloads the sim ready search query into the object spec.
Another motivation for this kind of separation of responsibility is that it'll easier to turn into a multi agent system with clear boundary.
wdyt?
There was a problem hiding this comment.
reasonable, updated
|
|
||
|
|
||
| async def _configure_asset_library(config: SimReadySearchConfig, traces: list[str]) -> Any | None: | ||
| try: |
There was a problem hiding this comment.
this is in our deps already shouldn't need to check import error
| await library.add_s3_source(config.s3_url or ISAAC_SIMREADY_GA_S3_URL) | ||
| elif config.source == SimReadySourceKind.SERVICE: | ||
| library.add_service_source(config.service_url or DEFAULT_SIMREADY_SERVICE_URL) | ||
| elif config.source == SimReadySourceKind.CACHE: |
There was a problem hiding this comment.
can we have just one sync bridge for library setup
from simready.search import AssetLibrary
library = AssetLibrary()
if config.source in (SimReadySourceKind.ISAAC_SIM_GA, SimReadySourceKind.S3):
asyncio.run(library.add_s3_source(config.s3_url or ISAAC_SIMREADY_GA_S3_URL))
elif config.source == SimReadySourceKind.SERVICE:
library.add_service_source(config.service_url or DEFAULT_SIMREADY_SERVICE_URL)
else:
# log error
return library
then we can drop all the awkward async from this file and clean it up significantly.
feels like we only need to support add_s3_source / add_service_source (remote)
There was a problem hiding this comment.
Im assuming we are bumping simready as P0, and dropping "bring you own assets from S3/local".
qianl-nv
left a comment
qianl-nv
left a comment
There was a problem hiding this comment.
Thanks for restructuring it! Add some nits. In particular, suggests clean up the EnvironmentGenerationAgent.traces usage hygiene to separate error vs info logs into separate dict entry/list under trances, then can clean up some of the ac-hoc logic in UI for trace display.
Approved, feel free to merge after address the comments.
|
|
||
| # Reading an asset means fetching it, so only the best few hits are worth checking before we | ||
| # give up on the phrase and let the agent fall back to the Arena asset registry. | ||
| ranked = matches[: max(MAX_INSPECTED_MATCHES_PER_PHRASE, max_results)] |
There was a problem hiding this comment.
why do we need both MAX_INSPECTED_MATCHES_PER_PHRASE and config.max_results_per_object?
can we just make sure config.max_results_per_object <=MAX_INSPECTED_MATCHES_PER_PHRASE when constructing the config? unless I misunderstand the purpose?
There was a problem hiding this comment.
The two numbers measure different things:
- max_results_per_object caps how many candidates are kept.
- The constant floors how many hits are inspected.
Updated names & docstring to clarify.
## Summary Add the simready-search dependency, to unblock #982 testing. ## Detailed description - Override boto3, botocore, requests and s3transfer to the versions isaacsim-kernel pins. simready-search asks for boto3==1.42.58, requests>=2.32.5 and s3transfer 0.16.x, all over-strict; without the overrides it does not resolve alongside either sim-stack flavor. --------- Signed-off-by: Xinjie Yao <xyao@nvidia.com>
…parts SimReady props give each part its own rigid body and hold the parts together with fixed joints, so a spray bottle reports two bodies. Isaac Lab expects one rigid body per rigid object and refuses to spawn them. - Add get_physics_structure() to group rigid bodies by fixed joints. - Use it in detect_object_type() so held-together bodies are RIGID and anything that can still move is ARTICULATION. - Add weld_usd_rigid_bodies() to write a cached asset that puts the rigid body on a parent prim and switches off the redundant joints. - Add apply_usd_variant_selections(), needed because SimReady props have no physics until the Physics variant is selected. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
The parts of a SimReady prop state their mass twice, once on the part and again on each of its collision shapes, and the two do not always agree. Merging the parts made PhysX read the shape figures instead of the part figures, which moved the disinfectant bottle's centre of mass from 0.052 m to 0.082 m. Add up the part figures during the merge and write them on the merged body, so the prop keeps the mass, centre of mass, and inertia it had before. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
Normalize prompts before spec inference, optionally query Isaac Sim GA SimReady props, and emit simready_usd_object candidates so generated YAML carries concrete USD paths without per-asset registry classes. Signed-off-by: Qian Lin <qianl@nvidia.com>
Keep prompts like "maple table" as background-only so the resting surface is the background asset itself rather than inventing an in-scene object_reference. Signed-off-by: Qian Lin <qianl@nvidia.com>
Select the authored Physics variant when spawning SimReady USD objects and when locating rigid bodies for contact sensors, preferring body prims when multiple rigid bodies share a depth. Signed-off-by: Qian Lin <qianl@nvidia.com>
OR path-token filters so phrases like "disinfectant bottle" still match assets whose USD paths contain only one of the words. Signed-off-by: Qian Lin <qianl@nvidia.com>
Searching for a "grey bin" returned a kitchen cabinet, because SearchFilterPathContains matches any substring and "bin" appears inside "Cabinets". The GA library has no bin at all, so the right answer is no result, which lets the generator fall back to the asset registry. - Keep only results that share a whole word with the search phrase, and order them by how many words match. - Split camelCase when reading a path, so sm_trashCan matches "can". - Merge multi-part SimReady props into one rigid body when spawning, and detect the object type with the Physics variant selected. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
A search for a "grey bin" came back with a kitchen cabinet, which carries a rigid body per door and cannot be picked up at all. An asset that is not one rigid body has no business in the objects of a generated environment. - Reject a search hit unless its USD holds exactly one RigidBodyAPI, and try the next hit in its place. - Match on the last word of the search phrase, which names the object itself, so a grey cabinet is no longer an answer for a grey bin. - Name the objects nothing was found for in the prompt and in the traces, so the agent falls back to the Arena asset registry and the user is told. - Drop the generated droid_disinfectant_into_grey_bin environment, whose bin was that cabinet. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
The SimReady search now turns down any asset with more than one rigid body, so nothing is left to merge. Remove the weld, the mass and inertia combining it needed, the joint-based classification that decided what to merge, and the tie-breaking that let contact sensors pick a body out of a multi-body prop. detect_object_type goes back to raising on several rigid bodies at the same depth, keeping only the variant selection SimReady props need. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
functools.cache cannot key on a dict of variants, which is the only reason the read was split in two. Keep the answers in a dict of its own instead, so the public function is the whole thing. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
The registry name is the asset library's own name, and the bucket and service URLs say where its assets come from, so the search module is not their home. It imports them instead, which also drops the deferred variants import. Signed-off-by: Xinjie Yao <xyao@nvidia.com>
57ed713 to
fe4179c
Compare
2 participants
Summary
Enable SimReady search API in agentic env gen
Detailed description
--enable_simready_searchon the environment generation runner, or the "SimReady search" expander in the review GUI.simready_droid_pick_place_cans_hammer_maple_table.yaml
Other examples
When Simready ones are not valid, fall back to the closest from Simready / Arena registry.