Repository navigation
Store non-tensor sample fields as one opaque value per sample (fix variable-length raw fields on the disk cache) - #1277
Conversation
litdata's BinaryWriter infers one serializer per leaf from the first sample it writes and zips later samples against it. A raw field whose length or shape varies between samples (notes per admission, measurement times, a None in one sample and a list in the next) therefore failed to write, was silently truncated, or was misread on the disk-backed path (set_task, create_sample_dataset(in_memory=False)). - SampleBuilder.encode keeps fields of processors that declare stores_tensor as tensors and pickles every other field, including keys not in any schema, into one bytes item per sample. - SampleDataset.__getitem__ decodes it, so index access, iteration, subset, split parts and dataloaders return exactly what went in. - FeatureProcessor.stores_tensor (default False) is set on processors whose process() always returns a tensor; a non-tensor value in such a field raises a ValueError naming the field. - The storage format is part of the processed-cache key, so caches written in the old layout are rebuilt instead of misread. - set_task sends the task to worker processes pickled, and raises a clear error if a worker cannot unpickle it, instead of hanging. - RawProcessor docstring states the contract; the float warning is gone. - Docs: "How Processed Samples Are Stored"; example examples/raw_fields_disk_cache.py. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
It appears this PR might break something in the CI here, that causes some part of the code to go from 6 minute unit tests to 6 hours here. Not entirely sure what or why. |
jhnwu3
left a comment
There was a problem hiding this comment.
Hey professor, something here breaks the CI. Not sure if you can take a look later.
The runtime of our unit tests is usually ~10 minutes, so it taking upwards of 6 hours and timing out suggests something else is up.
litdata.optimize starts helper processes even with num_workers=0, and when the transform raises it leaves them running. They hold multiprocessing's resource-tracker pipe open, so the next tracker shutdown (a later dataset build, or interpreter exit) waits forever. On Linux CI this hung the suite after test_misdeclared_tensor_processor_raises_with_field_name; reproduced with py-spy on ubuntu-latest (main process blocked in resource_tracker._stop_locked, two orphaned litdata _upload_fn workers). create_sample_dataset(in_memory=False) now writes with BinaryWriter in the calling process, as set_task's single-worker path does, so no processes start and errors propagate cleanly. A test asserts no process is started. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@jhnwu3 Thanks for flagging the 6-minute-to-6-hour CI slowdown. It was a hang, not slow tests, and this PR did trigger it. The cause is now found, and the fix is in 36b8c44. Cause. Master never hit this because no test raised inside How it was confirmed. Runs on ubuntu-latest with a watchdog and
Fix (36b8c44). CI on the fix (merged with current master): 1,473 core tests and 2 NLP tests pass, in a normal 8m20s, and contribution-rules passes. The earlier hang was in the test environment, not a problem with the storage change itself. |
jhnwu3
left a comment
There was a problem hiding this comment.
After a long session of reading documentation and digging, lgtm so far. Will keep an eye out in future PRs.
Problem
On the disk-backed path (
set_task,create_sample_dataset(in_memory=False)), litdata'sBinaryWriter.serializeflattens each sample and infers one serializer per leaf from the first sample only, thenzips later samples against it. Any non-tensor field whose length, shape or type varies between samples breaks:UnicodeDecodeErroror returns wrong values. This silent-corruption case was not tested before.Nonein one sample, a list in another: the write crashes.This hits
RawProcessorfields (notes per admission, measurement times),tuple_time_textwithout a tokenizer, StageNet's optional times,textwithNone, and any key that is not in a schema. For example, the multimodal mortality task (MultimodalMortalityPredictionMIMIC4: rawdischarge,radiology,lab_times) fails on master with 1 and 2 workers ('str' object has no attribute 'device').Fix
Every non-tensor field is stored as one opaque value per sample. Its layout can no longer vary, so litdata's first-sample inference holds for all samples.
SampleBuilder.encode: fields whose processor declaresstores_tensor = Truestay as tensors. Everything else, including non-schema keys, is pickled together into onebytesitem.SampleDataset.__getitem__decodes that item. Index access, iteration (shuffled or not),subset, split parts andget_dataloaderall go through it, and key order is preserved.FeatureProcessor.stores_tensor(defaultFalse) is set on processors whoseprocess()always returns a tensor.ImageProcessormakes it a property that followsto_tensor. A tensor-declared processor that returns a non-tensor raises aValueErrorthat names the field.storage_formatis added to the processed-cache key and tometadata, so caches in the old layout are rebuilt rather than misread.set_tasksends the task to pool workers pickled. A task that a worker can't unpickle (e.g. a class defined underif __name__ == "__main__") now raises aRuntimeErrorwith guidance instead of hanging the pool.RawProcessordocstring: documents the contract, the trust boundary (the cache already holds pickles inschema.pkl) and the ≤2.0.2 note. The float warning is removed, since floats now round-trip.docs/api/processors.rst. Example:examples/raw_fields_disk_cache.py.Processor audit
stores_tensor)to_tensor)Why the litdata cap (
>=0.2.64,<0.2.68) is kept, and why it's now safeThe fix doesn't depend on version-specific litdata behavior. Every sample now has the same leaf layout (fixed tensor fields plus one
bytesitem), which is the case litdata's first-sample inference handles correctly. I verified the round-trip on 0.2.64, 0.2.67 and 0.2.76, so lifting the cap later is a separate decision this PR doesn't block.Testing
tests/core/test_raw_storage.py:None, empty lists, varying flat str/int/float, nested, tuples, dicts with fixed and varying keys, lists withNone, a 100 KB string, bytes, numpy arrays.set_taskround-trip withnum_workers1 and 2, and withsplit=PatientSplit(...)parts.test_split_set_task: the cache-key test is updated, and a test checks that caches in the old layout are not reused.test_split_set_taskpass on litdata 0.2.64, 0.2.67 and 0.2.76.unittest discover, fresh cache): 1437 tests. All pass except 3 modules that needpytest, which isn't installed in that venv. Run under pytest, those modules give 31 passed and 3 failed intest_tuple_time_text_tokenizer. The same 3 fail on master: the HF tokenizer can't load in this environment.tools/check_pr_rules.pypasses.Notes
struct.errorindeserializewhenever a cachedtask_df.ldis re-read. On this machine, 3test_processor_schemastests hit this with the shared default cache, because this branch's new cache key forces a re-read; they pass with a fresh cache. It's out of scope here, but adding the litdata version to the cache key might be worth a separate change.collate_fn_dict_with_paddingtreats a list of 2-tuples as(time, value)pairs. That's existing behavior, now documented rather than changed.🤖 Generated with Claude Code