[Fix] Stop OvPhysX writing its collider cache into the Python interpreter directory - #6889
[Fix] Stop OvPhysX writing its collider cache into the Python interpreter directory#6889hujc7 wants to merge 15 commits into
Conversation
The kitless OvPhysX runtime bundles its own Carbonite, which resolves
the UJITSO derived-data cache location from ${omni_cache}. That token
is registered by omni.kit.app, so a run that never starts Kit cannot
resolve it, and Carbonite falls back to the directory holding the
Python interpreter.
Where that directory is writable the cache is silently written inside
the Python installation, shared across every project using that
interpreter and discarded on an interpreter upgrade. Where it is not
writable, startup logs "Failed to acquire exclusive lock to data
store" followed by "Failed to create local file data store".
Pass the location Kit itself uses so the cache lands in the Omniverse
user cache directory on both runtime branches.
…-derived-data-cache-dir
Greptile SummaryThe PR redirects kitless OvPhysX cooked-collider caches from the Python installation to the standard per-user Omniverse cache and updates the legacy-runtime fix so every supported constructor and device path receives an explicit cache configuration.
Confidence Score: 5/5The PR appears safe to merge. The previously reported legacy-path omission is resolved because the current code constructs and passes a cache-configured PhysXConfig for every supported legacy constructor and device combination, and no blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'upstream/d..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if "active_cuda_gpus" in physx_parameters and ovphysx_device == "gpu": | ||
| physx_kwargs["active_cuda_gpus"] = str(gpu_index) | ||
| physx_kwargs["config"] = ovphysx.PhysXConfig( |
There was a problem hiding this comment.
When the legacy runtime uses CPU physics or exposes the gpu_index constructor, this condition constructs PhysX without a PhysXConfig, leaving Carbonite's interpreter-relative cache fallback active and causing cache writes beside Python or omni.datastore errors when that directory is read-only.
There was a problem hiding this comment.
Confirmed and fixed in 48e4935.
The legacy branch now builds PhysXConfig unconditionally, so legacy CPU runs and the gpu_index constructor both receive the cache setting. The GPU-only /physics/suppressReadback and /physics/suppressFabricUpdate keys stay conditional, since those are correctly GPU-gated.
One correction worth recording: on that path the typed field was not merely omitted, it was unsafe. The declared runtime's PhysXConfigpredatescooked_collider_cache_dir(verified against an olderovphysxwheel:carbonite_overridespresent, the typed field absent), socooked_collider_cache_dir=would have raisedTypeErroron legacy GPU startup. The declared runtime now goes through the generic/UJITSO/datastore/localCachePath` override instead, which it accepts and which 0.5.9 rejects as conflicting with its typed field. The two routes are mutually exclusive by wheel version.
Test coverage went from 2 to 6 combinations (two legacy constructors x two devices, plus the current runtime x two devices), and each stubbed PhysXConfig now mirrors its runtime's real signature, so a keyword the real runtime would reject fails the test instead of being absorbed by a permissive lambda **kwargs`.
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The PR correctly routes the OVPhysX cooked-collider cache through the typed PhysXConfig field and adds a changelog fragment and regression coverage, but the legacy runtime path still omits this configuration for CPU runs and constructors using gpu_index.
- Design and architecture: Containing cache-path resolution within
OvPhysxManageris appropriate, and the platform-specific user-cache mapping avoids writing beside the interpreter. However, the legacy-runtime integration applies the cache configuration only within one GPU-specific constructor branch rather than as a runtime-wide setting. - API: No public Isaac Lab API symbols are added, removed, or renamed. The private helper and internal construction changes do not require deprecation or documentation exports, and the package changelog fragment follows the repository format.
- Implementation: The current runtime path always passes
cooked_collider_cache_dir, but the legacy path constructsPhysXConfigonly whenactive_cuda_gpusis exposed and GPU physics is selected. Legacy CPU runs and thegpu_indexconstructor variant therefore retain the interpreter-relative fallback. Construct the cache-bearing configuration unconditionally for the legacy path while keeping GPU selectors and GPU-only overrides conditional; extend the regression test to cover those paths.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| if "active_cuda_gpus" in physx_parameters and ovphysx_device == "gpu": | ||
| physx_kwargs["active_cuda_gpus"] = str(gpu_index) | ||
| physx_kwargs["config"] = ovphysx.PhysXConfig( | ||
| cooked_collider_cache_dir=OvPhysxManager._cooked_collider_cache_dir(), |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Legacy runtime gets cache dir only on GPU
On the legacy runtime path, PhysXConfig is constructed only when the constructor exposes active_cuda_gpus and the device is "gpu". For legacy CPU runs, or constructors taking gpu_index, ovphysx.PhysX is created without any config, so Carbonite keeps its interpreter-relative cache fallback and the reported behavior persists. Build the cache-bearing config unconditionally in this branch and keep the GPU-specific overrides and selector conditional.
There was a problem hiding this comment.
Confirmed and fixed in 48e4935, along the lines you suggested.
The cache-bearing config is now built unconditionally on the legacy path, with the GPU selectors (active_cuda_gpus / gpu_index) and the GPU-only /physics/suppress* overrides kept conditional:
legacy_overrides = {"/UJITSO/datastore/localCachePath": OvPhysxManager._cooked_collider_cache_dir()}
if "active_cuda_gpus" in physx_parameters and ovphysx_device == "gpu":
physx_kwargs["active_cuda_gpus"] = str(gpu_index)
legacy_overrides["/physics/suppressReadback"] = True
legacy_overrides["/physics/suppressFabricUpdate"] = True
elif "gpu_index" in physx_parameters:
physx_kwargs["gpu_index"] = gpu_index
physx_kwargs["config"] = ovphysx.PhysXConfig(carbonite_overrides=legacy_overrides)One deviation from the suggestion: the declared runtime cannot take the typed field at all. Its PhysXConfig predates cooked_collider_cache_dir (verified against an older ovphysx wheel), so the typed keyword would raise TypeError there rather than merely being skipped. It therefore uses the generic /UJITSO/datastore/localCachePath override, while the current runtime keeps the typed field, which is the only route it accepts (0.5.9 rejects the generic key as conflicting with the typed field).
The regression test was extended as you asked, to two legacy constructors x two devices plus the current runtime x two devices, and its stubs now mimic each runtime's real PhysXConfig` signature so this class of mistake fails the test.
The declared runtime's PhysXConfig predates cooked_collider_cache_dir, so passing that keyword raised TypeError there. The config was also built only for GPU runs exposing active_cuda_gpus, leaving legacy CPU runs and the gpu_index constructor on the interpreter-relative fallback the previous commit set out to remove. Route the declared runtime through the generic /UJITSO/datastore/localCachePath override, which it accepts and the current runtime rejects as conflicting with its typed field, and build the config unconditionally so both devices and both constructors receive it. Fall back to the temporary directory when the home directory cannot be resolved: expanduser leaves "~" unchanged, and a relative path would put the cache under the working directory. Give each stubbed PhysXConfig the signature of the runtime it stands for, so a keyword the real runtime would reject fails the test instead of being absorbed.
…-derived-data-cache-dir
Both construction sites spelled out which keyword carries the cache location, so the rule for choosing between the typed field and the generic override lived in two places and had to be kept in step. Move it into a single helper that inspects the runtime's PhysXConfig signature and writes whichever keyword that signature accepts, merging into any overrides the caller already set. Selecting on the config signature rather than on which PhysX API is in use also covers a runtime whose config predates the typed field while its PhysX class does not, which the previous branch-based choice could not express.
…-derived-data-cache-dir
Having the helper decorate a caller-supplied dict meant both call sites had to name and hold that dict purely to hand it over and splat it back, so each grew a variable describing nothing the reader needs. Build and return the PhysXConfig instead. The construction sites become expressions again and the two intermediate dicts go away, leaving only the carbonite overrides the legacy branch genuinely assembles.
The helper that chose between the typed field and the generic override guarded a runtime whose PhysXConfig and PhysX class disagree about the typed field. No published wheel does: in every version available, the field and PhysX.set_cpu_mode appear together or not at all. The branch already tells the two runtimes apart, so each can name the keyword its own config accepts. Drop the helper and construct PhysXConfig directly at both sites, and name the path helper for what it returns rather than for what one of its two callers does with it.
…-derived-data-cache-dir
|
@greptileai review |
| @staticmethod | ||
| def _derived_data_cache_dir() -> str: | ||
| """Return the canonical Omniverse derived-data cache directory. | ||
|
|
||
| Mirrors the location Kit resolves from its ``${omni_cache}`` token. The kitless | ||
| OVPhysX runtime cannot resolve that token, and its Carbonite fallback resolves | ||
| relative to the Python interpreter binary, which is not writable on installs that | ||
| reuse a system interpreter. | ||
| """ | ||
| if sys.platform == "win32": | ||
| base = os.environ.get("LOCALAPPDATA") or os.path.join(os.path.expanduser("~"), "AppData", "Local") | ||
| cache_dir = os.path.join(base, "ov", "cache", "DerivedDataCache") | ||
| else: | ||
| base = os.environ.get("XDG_CACHE_HOME") or os.path.join(os.path.expanduser("~"), ".cache") | ||
| cache_dir = os.path.join(base, "ov", "DerivedDataCache") | ||
| if not os.path.isabs(cache_dir): | ||
| # ``expanduser`` leaves "~" unchanged when the home directory cannot be resolved | ||
| # (e.g. an arbitrary-UID container with no passwd entry); a relative path would | ||
| # put the cache under the working directory. | ||
| cache_dir = os.path.join(tempfile.gettempdir(), "ov", "DerivedDataCache") | ||
| return cache_dir |
There was a problem hiding this comment.
Should we use this, if IsaacSim is not installed does this even exists?
| # ``expanduser`` leaves "~" unchanged when the home directory cannot be resolved | ||
| # (e.g. an arbitrary-UID container with no passwd entry); a relative path would | ||
| # put the cache under the working directory. | ||
| cache_dir = os.path.join(tempfile.gettempdir(), "ov", "DerivedDataCache") |
There was a problem hiding this comment.
🤖 AI-generated review: This fallback uses the same predictable <temp>/ov/DerivedDataCache directory for every user. On a shared Linux host, the first user to create /tmp/ov will normally own it, so another arbitrary-UID process may be unable to write the cache and hit the same datastore failure this branch is intended to avoid; another user can also pre-populate that cache. Could we use a securely created temporary directory (for example, tempfile.mkdtemp(...)) or another safely per-user location here?
|
|
||
|
|
||
| @pytest.mark.skipif(sys.platform == "win32", reason="cache root is LOCALAPPDATA-derived on Windows") | ||
| def test_manager_passes_explicit_derived_data_cache_dir(monkeypatch, tmp_path): |
There was a problem hiding this comment.
🤖 AI-generated review: The regression is worth covering: the neighboring runtime-API tests do not assert the cache setting, and a focused test would have caught the earlier legacy-CPU omission. The full 3 × 2 matrix seems larger than the distinct behavior, though, while the decorator skips the only platform-specific branch added by this PR. Could we fold strict PhysXConfig fakes and cache assertions into the existing current/legacy API tests, retain focused cases for legacy CPU and gpu_index, and make the expectation conditional on LOCALAPPDATA versus XDG_CACHE_HOME instead of skipping Windows? That preserves the useful regression coverage with less duplicated fixture code.
Develop removed the pre-0.5.9 OvPhysX compatibility code in 2404010 (Remove obsolete OVPhysX bootstrap hacks, isaac-sim#6074), so the legacy half of this branch no longer has a runtime to configure. Resolve in develop's favour and keep only the cooked-collider cache setting: - drop the legacy PhysX branch and its /UJITSO/datastore/localCachePath override; the pinned runtime takes the typed cooked_collider_cache_dir - restore the "import os" that the auto-merge dropped along with develop's removal of os._exit, since _derived_data_cache_dir still needs it - fold the cache assertion and a strict PhysXConfig fake into develop's parametrized test_manager_supports_pinned_runtime_api and delete the standalone runtime matrix, whose legacy stubs no longer match any code
The fallback fires when no absolute home-based path resolves, and it used a fixed <temp>/ov directory. On a shared POSIX host the temp root is world-writable, so the first user to create it owns it and every other UID either cannot write the cache or reads collider data that user placed there. Windows already gives each user a private temp directory, so the UID suffix is applied only where os.getuid exists.
Follow the pattern the sibling kitless runtime already uses: OVRTXRendererCfg declares log_file_path with a temp-rooted default and threads it into RendererConfig. The cache directory now lives on OvPhysxCfg the same way, replacing the bespoke XDG_CACHE_HOME/LOCALAPPDATA resolution, which was the only such code in source/. The default is per-user (<temp>/ovphysx_derived_data_cache_<uid>) because the POSIX temp root is shared: a fixed name is created 0755 by whoever runs first, so a second user cannot write it and hits the omni.datastore failure this branch removes. Windows has no getuid and already gives each user a private temp root, so the suffix is applied only where it exists. _create_physx_instance takes the directory as an argument, and the caller falls back to the module default because the runtime is also constructed outside a configured simulation. Both PhysXConfig fakes gain the field so they mirror the real 0.5.9 signature instead of diverging from it.
…-derived-data-cache-dir
2 participants
1. Summary
physics=ovphysxruns wrote their UJITSO derived-data cache into the directory holding the Python interpreter. The location is now a config field, defaulting to a per-user directory under the system temp directory./usr/bin/python3.12), every run logged two[Error] [omni.datastore]lines.2. Root cause
The
ovphysxwheel bundles its own Carbonite runtime, which resolves the UJITSO cache fromrealpath("/proc/self/exe")+/cache/DerivedDataCachewhen no path is configured.That assumption — the executable's directory belongs to me — holds for Kit, whose
kitbinary lives in its own application package next to acache/sibling (on a local Isaac Sim source build,_isaac_sim/kit/cache/DerivedDataCache). It breaks for every embedder, where the "executable" is a shared Python interpreter whose directory the program does not own.3. Fix
OvPhysxCfg.cooked_collider_cache_dirselects the directory, and_create_physx_instancepasses it toovphysx.PhysXConfig(cooked_collider_cache_dir=...). This mirrors the sibling kitless runtime:OVRTXRendererCfg.log_file_pathdeclares a temp-rooted default and is threaded intoRendererConfig.Routing the same key through the generic
carbonite_overridesdict is rejected by the wheel at runtime —conflicts with typed field 'cooked_collider_cache_dir'. Use the typed field instead.— so the typed field is the only accepted route.The default is per-user,
<temp>/ovphysx_derived_data_cache_<uid>. A fixed name would be created0755by whichever user runs first, leaving a second user on the same host unable to write it and hitting the very failure above. Windows has nogetuidand already gives each user a private temp root, so the suffix is applied only where it exists. Sites wanting one shared cache point the config field at a directory they provision.4. Validation
Run three ways, varying only the fix and whether the interpreter directory is writable:
omni.datastoreerrorsbin/cache/DerivedDataCacheArm A1 reproduces both reported lines verbatim. Arm A0 is the one showing this is a wrong-location bug rather than a permissions bug: no error, cache still in the wrong place. All three arms exit 0 with training completing, matching the report that functionality is unaffected.
5. Test
The cache assertion and a strict
PhysXConfigfake fold into the existing parametrizedtest_manager_supports_pinned_runtime_api, which already covers CPU and GPU; both parametrizations fail when the keyword is removed.test_ovphysx_manager_lifecyclecovers construction with no config, where the module default applies. Both fakes carrycooked_collider_cache_dirso they mirror the real 0.5.9 signature rather than diverging from it.6. Notes
tempfile.mkdtemp's unpredictability. Hardening against a pre-created directory was judged out of scope; the same exposure exists forGIT_ASSET_CACHE_DIR.ovrtxexposes no equivalent knob (RendererConfig's string keys are a closed enum: log path, log level, CUDA devices). Only one error appeared in the report and it came from the ovphysx bring-up, so this is expected to close it, but anovrtx-side store would need a wheel-side change.