Switch assets.py omni.client calls to async#5553
Conversation
dengyuchenkit
requested review from
Mayankm96,
hhansen-bdai and
kellyguo11
as code owners
github-actions
Bot
added
bug
Greptile SummaryThis PR replaces four synchronous
Confidence Score: 5/5The change is safe to merge — it replaces blocking sync Nucleus calls with async equivalents using a well-scoped helper, and all six app configs are updated to carry the required extension dependency. The async migration is mechanically straightforward: each replaced call maps 1-to-1 onto its source/isaaclab/isaaclab/utils/assets.py — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_drive_kit_async(coro)"] --> B{"Kit app running?"}
B -- "No" --> C["asyncio.run(coro)"]
B -- "Yes" --> D["import omni.kit.async_engine"]
D -- "OK" --> E["run_coroutine(coro) → task"]
D -- "Exception" --> F["ensure_future(coro) → task"]
E --> G["while not task.done(): app.update()"]
F --> G
G --> H["task.result()"]
C --> I["return result"]
H --> I
Reviews (2): Last reviewed commit: "Switch assets.py omni.client calls to as..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| app = _kit_app() | ||
| if app is None: | ||
| return asyncio.get_event_loop().run_until_complete(coro) |
There was a problem hiding this comment.
asyncio.get_event_loop().run_until_complete() call with asyncio.run(), which is safe in Python 3.10+ and always creates a fresh, clean event loop for the non-Kit path.
| app = _kit_app() | |
| if app is None: | |
| return asyncio.get_event_loop().run_until_complete(coro) | |
| app = _kit_app() | |
| if app is None: | |
| return asyncio.run(coro) |
There was a problem hiding this comment.
Updated in latest commit.
| try: | ||
| import omni.kit.async_engine # noqa: PLC0415 | ||
|
|
||
| task = omni.kit.async_engine.run_coroutine(coro) | ||
| except Exception: | ||
| task = asyncio.ensure_future(coro) | ||
| while not task.done(): | ||
| app.update() | ||
| time.sleep(0) |
There was a problem hiding this comment.
asyncio.ensure_future() can cause an infinite loop
The broad except Exception catches any failure from importing or calling omni.kit.async_engine.run_coroutine(). When the import fails (e.g., headless kit configs that don't load omni.kit.async_engine — all five other .kit files in apps/ are missing this dependency), execution falls through to asyncio.ensure_future(coro). In synchronous context on Kit's main thread there is no running asyncio loop, so ensure_future schedules the task on whatever loop asyncio.get_event_loop() returns — which is not the loop that app.update() pumps. The while not task.done() loop then spins forever, hanging the application. isaaclab.python.headless.kit (the most common training/CI variant) does not list omni.kit.async_engine and will reliably hit this path.
There was a problem hiding this comment.
- This PR adds async_engine to all apps/*.kit files, so calling async_engine won't hit fallback and won't fail in all configs.
- Also removed the try/except so if async_engine is not imported, it fails loudly instead of silently hits fallback.
There was a problem hiding this comment.
Thanks for the clarification! I see "omni.kit.async_engine" = {} is now added to all the apps/*.kit files, which ensures the extension is loaded in all supported configurations.
The fallback to asyncio.ensure_future is still there as defensive code, but it should no longer be hit since async_engine is now guaranteed to be available. This looks good to me. 👍
There was a problem hiding this comment.
This PR is updated and removed the fallback to asyncio.ensure_future.
There was a problem hiding this comment.
Great, that's cleaner. Thanks for the update!
The four sync calls (stat, copy, read_file) in check_file_path, retrieve_file_path, and read_file parked Kit's main thread on every Nucleus round-trip and triggered Kit's "Detected a blocking function" warning during env init. They now run through a new _drive_kit_async helper, which schedules the *_async coroutine on Kit's asyncio loop and ticks app.update() until it resolves -- same wall time, but Kit keeps rendering during the wait. Add omni.kit.async_engine to every IsaacLab .kit file so its observer steps the asyncio loop each Kit frame; without it the spin loop would deadlock (headless and CI runs would hang on any Nucleus I/O). The no-Kit fallback uses asyncio.run for Python 3.12+ compatibility.
dengyuchenkit
force-pushed
the
ydeng/fix-omniclient-blocking
branch
from
bb17e09 to
3838492
Compare
dengyuchenkit
changed the title
The try/except around omni.kit.async_engine in _drive_kit_async would schedule the coroutine onto asyncio.get_event_loop() if the import or run_coroutine call raised. That loop is never pumped by app.update(), so the spin loop would silently hang forever. All six bundled apps/*.kit files declare the dependency, so the fallback isn't reachable from IsaacLab itself. But a stripped or forked downstream .kit could drop it, turning every Nucleus I/O into an unrecoverable freeze with no traceback. Let the import raise instead. Custom configs missing the dependency now fail immediately with a clear ImportError pointing at the cause.
1 participant
The four sync calls (stat, copy, read_file) in check_file_path, retrieve_file_path, and read_file parked Kit's main thread on every Nucleus round-trip and triggered Kit's "Detected a blocking function" warning during env init.
They now run through a new _drive_kit_async helper, which schedules the *_async coroutine on Kit's asyncio loop and ticks app.update() until it resolves -- same wall time, but Kit keeps rendering during the wait.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there