Skip to content

Replaces fcntl with filelock to avoid windows import issues#5544

Merged
kellyguo11 merged 2 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix/from-files-windows-filelock
May 8, 2026
Merged

Replaces fcntl with filelock to avoid windows import issues#5544
kellyguo11 merged 2 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix/from-files-windows-filelock

Conversation

@AntoineRichard
Copy link
Copy Markdown
Collaborator

Description

The isaaclab.sim.spawners.from_files module imports fcntl unconditionally at module load. fcntl is Unix-only, so on Windows the import fails with:

ModuleNotFoundError: No module named 'fcntl'

This breaks any Windows usage of the spawner — including single-GPU runs that never take the lock path. Reproducer reported by a user (IsaacLab develop @ b258e87, IsaacSim 6.0.0rc41):

python scripts/reinforcement_learning/rl_games/train.py --task=Isaac-Cartpole-v0 --headless --video --video_length 100 --video_interval 500 --max_iterations 5

fcntl was introduced in #5032 to serialize USD download/stage composition across distributed ranks (preventing segfaults in Sdf_CrateFile::_MmapStream::Read on shared cached USD files). The intent is correct — only the implementation is Unix-only.

Change

  • Replace fcntl.flock with filelock.FileLock, which uses fcntl on POSIX and msvcrt on Windows.
  • Use contextlib.nullcontext for the single-rank path so the lock file is only created when actually needed (i.e., LOCAL_WORLD_SIZE > 1).
  • Drop the manual try/finally and the # noqa: SIM115 on the bare open(...) — the with FileLock(...) form is exception-safe.
  • Declare filelock in source/isaaclab/setup.py::INSTALL_REQUIRES. It was previously only transitively available via transformershuggingface_hub; making it a direct dep so we don't depend on a transitive chain that could change.

The lock semantics are unchanged: an exclusive advisory lock on <tempdir>/isaaclab_usd_spawn.lock, held only while LOCAL_WORLD_SIZE > 1.

Original lock implementation: @ooctipus (#5032) — tagging for review since this changes the locking primitive.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

N/A — import-time failure on Windows; no UI surface.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation (N/A — internal change)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works — the failure is import fcntl at module load on Windows; a regression test would require a Windows CI runner, which IsaacLab does not currently exercise. Locally verified the module imports and FileLock resolves to filelock._unix.UnixFileLock on Linux (and would resolve to WindowsFileLock on Windows).
  • I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump extension.toml — CI handles that)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

The from_files spawner imported `fcntl` unconditionally at module load
to serialize USD downloads across distributed ranks. `fcntl` is
Unix-only, so importing the spawner on Windows raised
`ModuleNotFoundError: No module named 'fcntl'` even for single-rank
runs that never take the lock path.

Switch to `filelock.FileLock`, which selects `fcntl` on POSIX and
`msvcrt` on Windows, and use `contextlib.nullcontext` for the
single-rank case so the lock is allocated only when needed. Declare
`filelock` in `install_requires` — it was previously available only
as a transitive of `transformers`/`huggingface_hub`.
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels May 8, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 8, 2026

Greptile Summary

This PR fixes a Windows import failure in isaaclab.sim.spawners.from_files by swapping the Unix-only fcntl module for the cross-platform filelock library, and explicitly adding filelock to INSTALL_REQUIRES.

  • _spawn_from_usd_file now uses FileLock (blocking, exclusive) when LOCAL_WORLD_SIZE > 1 and nullcontext() otherwise, preserving the original serialization semantics while avoiding lock-file creation on single-rank runs.
  • try/finally manual unlock is removed in favour of the context manager's __exit__, which is exception-safe and cleaner.

Confidence Score: 5/5

The change is a minimal, well-scoped replacement of a Unix-only primitive with an equivalent cross-platform one; no behavioral differences are introduced on existing POSIX systems.

Both the locking path (FileLock — blocking, exclusive by default, released via __exit__) and the no-op path (nullcontext) correctly mirror the original code's intent. Exception safety is actually improved over the manual try/finally. The only loose end is the absence of a version floor on filelock, but the library has a stable API and this matches the style of other entries in INSTALL_REQUIRES.

No files require special attention; all three changed files are straightforward.

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/spawners/from_files/from_files.py Replaces Unix-only fcntl.flock with cross-platform filelock.FileLock; uses nullcontext for single-rank path — logic and exception safety are preserved.
source/isaaclab/setup.py Adds filelock as an explicit install requirement; no version constraint specified (matches the style of most other entries in the list).
source/isaaclab/changelog.d/antoiner-fix-from-files-windows-filelock.rst New changelog fragment documenting the Windows import fix and the added filelock dependency.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[_spawn_from_usd_file called] --> B{LOCAL_WORLD_SIZE > 1?}
    B -- Yes --> C[lock = FileLock\nisaaclab_usd_spawn.lock]
    B -- No --> D[lock = nullcontext]
    C --> E[with lock: acquire exclusive file lock]
    D --> E
    E --> F{file_status == 2?}
    F -- Yes --> G[retrieve_file_path\nforce_download=False]
    F -- No --> H[get_current_stage]
    G --> H
    H --> I{prim exists at prim_path?}
    I -- No --> J[create_prim]
    I -- Yes --> K[log warning]
    J --> L[release lock / exit context]
    K --> L
    L --> M[modify variants, rigid body,\ncollision, mass props...]
Loading

Reviews (1): Last reviewed commit: "Fix from_files spawner import on Windows" | Re-trigger Greptile

The docs CI installs only `docs/requirements.txt`, which omits
`filelock`. Sphinx autodoc therefore fails to import the
`from_files` module (which now uses `filelock.FileLock`), producing
three "failed to import" warnings that are treated as errors.

Add `filelock` to `autodoc_mock_imports` alongside the other small
utility libraries already mocked there.
@AntoineRichard AntoineRichard requested a review from kellyguo11 as a code owner May 8, 2026 11:29
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label May 8, 2026
@AntoineRichard AntoineRichard changed the title Fix from_files spawner import on Windows (replace fcntl with filelock) Replaces fcntl with filelock to avoid windows import issues May 8, 2026
@kellyguo11 kellyguo11 merged commit 4687c34 into isaac-sim:develop May 8, 2026
52 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants