Accept pathlib.Path in checkpoint I/O helpers - #2193
Conversation
Signed-off-by: Ethan Liu <ethantsliu@gmail.com>
Confidence Score: 5/5The PR appears safe to merge with no actionable defects identified. PathLike values are normalized before string-specific operations, existing string behavior remains intact, and the new tests exercise the intended local and directory-download paths.
|
| Filename | Overview |
|---|---|
| skyrl/backends/skyrl_train/utils/io/io.py | Consistently normalizes PathLike inputs before existing string and fsspec operations without changing behavior for current string callers. |
| tests/backends/skyrl_train/utils/test_local_read_files.py | Adds focused regression tests covering Path inputs for local context managers and directory download destinations. |
Reviews (1): Last reviewed commit: "Accept pathlib.Path in checkpoint I/O he..." | Re-trigger Greptile
There was a problem hiding this comment.
Code Review
This pull request updates the file I/O utilities in skyrl/backends/skyrl_train/utils/io/io.py to support both str and os.PathLike[str] (such as pathlib.Path objects) for local filesystem paths. This is achieved by updating the type annotations and using os.fspath() to normalize the path arguments before processing. Additionally, corresponding unit tests have been added to verify this new behavior. There are no review comments, and I have no additional feedback to provide.
Accept pathlib.Path in checkpoint I/O helpers
Checkpoint I/O helpers fail on
pathlib.Pathinputs because cloud-path detection expects a string. Directory downloads also pass localPathdestinations into fsspec code that expects strings.Accept
str | os.PathLike[str]for local paths and normalize them withos.fspath(). Directory context managers still yield strings, and cloud URIs are still supplied as strings.The focused filesystem tests pass (4 tests, including 2 new regression tests). Both new tests fail against the original implementation. Cloud downloads are tested with an in-memory filesystem.
Part of #252.
Note
Low Risk
Behavior-preserving type widening and string normalization for local paths; cloud URI handling is unchanged aside from safer local-path coercion.
Overview
Checkpoint and filesystem helpers in
io.pynow acceptstr | os.PathLike[str]for local paths instead of strings only. Each entry point normalizes inputs withos.fspath()so cloud URI detection no longer breaks onpathlib.Path, and fsspec/localos.makedirsalways receive string paths. Cloud source/destination URIs stay typed asstr.Context managers such as
local_work_dir,local_read_dir, andlocal_read_filesstill yield strings after normalization. Coverage adds regressions forPathwith local work/read flows and fordownload_directoryinto aPathdestination using an in-memory fsspec filesystem.Reviewed by Cursor Bugbot for commit 8aff1ab. Bugbot is set up for automated code reviews on this repo. Configure here.