Fixes by Fable and Codex#84
Merged
takeshi-yoshimura merged 10 commits intoJun 12, 2026
Merged
Conversation
get_multi_cols() resolved each tensor's (rank, lidx) but then indexed rank_loaders with `lidix`, a leftover loop variable holding the last file's index. When the requested tensors spanned multiple files with auto_mem_delete enabled, the instantiated-count check compared against the wrong file's tensor count and free_dev_ptrs() could release a device buffer that was still in use (or leave the right one unfreed). Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The negative index/bound conversion added an extra +1 (shape + idx + 1), so frame[-1] produced an out-of-range offset that silently addressed the neighboring tensor's bytes, and slices like [1:-1] kept one element too many. The element count also used floor division (length // step), undercounting strided slices such as [0:5:2]. Normalize slice bounds with slice.indices(), which matches Python sequence semantics for None/negative/out-of-range values and negative steps, and compute the length with ceiling division. Also iterate the remaining dims from len(val) so an empty tuple index no longer raises NameError on the undefined loop variable. Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two bugs fixed in the previous commits survived because no test exercised these paths: - test_tensor_frame_getitem checks TensorFrame.__getitem__ against Python list slicing for all integer indices and an exhaustive bound/step grid, including negative indices, strided slices, the empty tuple, and error cases. - test_get_multi_cols_multi_file_auto_free spans get_multi_cols() across two files with auto_mem_delete forced on and asserts that exactly the fully-instantiated file's device buffer is freed. Both tests fail on the previous code and pass with the fixes. Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
as_dict() dropped self.rank_loaders entirely when auto_mem_delete was enabled, which made the subsequent close() a no-op. Files whose tensors were not all requested through as_dict() kept their device buffers allocated until process exit. Keep rank_loaders intact; close() and the per-file auto-free accounting already release everything exactly once (free_dev_ptrs is idempotent). Add a regression test that requests a subset of one file's tensors via as_dict() and asserts close() returns the memory accounting to zero. Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
from_fd() assumed a single os.read() returns all requested bytes; a short read silently truncated the header length or the header JSON and surfaced as a confusing parse error. Read through a _read_exact() helper that loops until the requested byte count is complete and raises a clear UnexpectedEOF error otherwise. Also close the descriptor in from_file() with try/finally so a parse failure no longer leaks the fd. Add a regression test that parses a header through an os.read() that returns at most 3 bytes per call. Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GdsFileCopier.submit_io() ignored the return value of submit_read, so a submission failure was recorded as a bogus negative request id and only surfaced later as an undefined wait. Raise immediately, matching the nogds copier. NoGdsFileCopier.wait_io() raised on the first failed request, leaking the file descriptor and leaving later requests in flight against it. Drain every request first, close the fd, then report all failures. Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
copier/unified.py imported torch at module level for mmap+pin_memory, cuda synchronization, and device-name detection. Since copier/__init__ and loader.py import this module unconditionally, the whole package required torch even for paddle users, violating the rule that framework-specific code lives behind the frameworks abstraction. Add the missing primitives to the frameworks layer and route the copier through them: - TensorBase.data_ptr() - FrameworkOpBase.synchronize(device): no-op default, cuda sync in the torch/paddle backends - FrameworkOpBase.get_device_name(index): "" default for platform detection - FrameworkOpBase.mmap_file_pinned(filename, length, offset): implemented for torch (from_file + pin_memory); other frameworks raise NotImplementedError until they support the unified copier is_unified_memory_system() now takes the framework op to query the device name; the loader passes its framework into the copier constructor factories so the gds->unified fallback can use it. With no framework available, only the FASTSAFETENSORS_UNIFIED_MEM override can enable it. Add a policy test that rejects module-level torch imports anywhere outside frameworks/_torch.py and the torch-specific parallel_loader. Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
parallel_loader.py imported torch at module level only to query the global distributed rank in enable_tqdm() and for type hints. Since the package __init__ imports this module (via ParallelLoader/AutoLoader), the whole package still required torch even for paddle users. Add FrameworkOpBase.get_global_rank() (0 by default, torch.distributed / paddle.distributed in the backends) and pass the loader's framework into enable_tqdm(); type hints become framework-agnostic Any. With this, frameworks/_torch.py is the only module importing torch at module level, and the package now imports cleanly with torch blocked -- verified with a meta-path import hook. Tighten the policy test allowlist accordingly. Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Treat pg=None as single-process mode inside PipelineParallel so ParallelLoader(None, ...) and AutoLoader(None, ...) can construct batches without dereferencing a missing process group. Add a regression test for the single-process constructor path. Co-Authored-By: Codex <noreply@openai.com> Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com>
Generate a tiny GPT-2-shaped safetensors fixture locally by default so unit tests do not fail due to Hugging Face rate limits or network availability. Keep the real GPT-2 download path available behind FASTSAFETENSORS_TEST_USE_HF_GPT2=1 for explicit integration-style runs. Co-Authored-By: Codex <noreply@openai.com> Signed-off-by: Takeshi Yoshimura <tyos@jp.ibm.com>
takeshi-yoshimura
merged commit 067e405
into
foundation-model-stack:main
13 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.