feat(serverless): network-volume warm cache (VolumeCache) with adaptive transport - #538
feat(serverless): network-volume warm cache (VolumeCache) with adaptive transport#538deanq wants to merge 63 commits into
Conversation
|
Capy auto-review is paused for this organization because the usage-cycle auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
…docs Folds in runpod/runpod-python#538 (SLS-367): documents the additive max_workers constructor argument and the size-bucketed transport (small.tar + parallel big/ copy + versioned manifest). Also reconciles the page to the shipping VolumeCache API: the earlier env-var toggles (RUNPOD_VOLUME_CACHE/_MAX_GB/CACHE_DIRS), the warm() method, max_size_gb, and automatic worker-loop wiring were removed upstream in favor of the single explicit opt-in context manager.
|
Promptless prepared a documentation update related to this change. Triggered by PR #538 (adaptive size-bucketed transport) Docs handled via the existing VolumeCache draft (docs PR #693) rather than a new PR. Folded in this PR's user-facing delta — the additive Review: docs PR #693 |
…test state Wrap build_default_cache() parse+construct in try/except to prevent malformed RUNPOD_VOLUME_CACHE_MAX_GB from crashing worker startup; degrades to cache-disabled instead. Add sync() no-op to fake cache in test_run_worker_hydrates_registered_cache and wrap test body with reset_builtin_state_for_test() to prevent module-state leakage to subsequent tests (sync_after_job could fire and hit AttributeError).
…e, namespace validation, lint
… open() in assert, unused global)
- Remove the unplanned "._" AppleDouble basename skip from the production extractor; it would silently drop legitimate files on a real worker. The macOS bsdtar artifact it worked around is now handled in the affected tests by monkeypatching _tar_binary to force deterministic pure-Python tarfile packing. - Guard tf.getmembers() so a truncated/corrupt small.tar can no longer raise out of _extract_small; return the count extracted so far instead. Add a regression test that truncates a valid archive mid-body and asserts no exception propagates. - Drop the redundant os.path.realpath(dst)/os.makedirs additions in _extract_small; TarFile.extract already creates parent directories and _is_safe_dest already resolves the path internally, matching the brief's structure.
Replace archive-truncation test with a mocked tarfile.open whose getmembers() raises tarfile.TarError, deterministically exercising the inner corrupt-member-listing guard instead of the outer open()-time guard (truncation offset unreliably hit the outer path instead).
…o big/ Addresses Copilot review on PR #538: - _copy_parallel streams the success count from pool.map instead of materializing a per-file results list for large trees. - _do_hydrate confines each big-bucket source path inside big/ (new _within guard) so a crafted manifest entry can't read outside the mirror; the destination was already guarded by _is_safe_dest.
|
I think I have the same question as with #531, why not focus on the configurable mounts so that users can just mount volumes where they need them instead of copying files on every request (if they select refresh worker, which many do, you will have to re-copy every request) |
d261f73 to
ce6adfe
Compare
…ache-adaptive-transport
A bytes dirs entry (e.g. dirs=b"/root/.cache") passed os.fspath unchanged, so os.walk yielded bytes names and _is_safe_dest compared bytes against str prefixes, raising TypeError that best-effort mode silently swallowed. Decode each entry with os.fsdecode so all _dirs are text. Add a regression test. Addresses review feedback on #531 (carried here as the superseding PR).
deanq
changed the title
| self._volume_path = os.fspath(volume_path) | ||
| self._best_effort = best_effort | ||
| self._max_workers = max_workers or min(32, (os.cpu_count() or 4) * 4) |
|
|
||
| big_pairs = [(f, os.path.join(self._big_root, os.path.relpath(f, "/"))) for f in big_files] | ||
| big_copied = self._copy_parallel(big_pairs) | ||
| big_meta = [m for m in (self._file_meta(f) for f in big_files) if m] |
There was a problem hiding this comment.
big_meta is built from every local source even when _copy_parallel() fails to create or update its destination. The manifest is then written as the commit marker for a supposedly complete mirror. A disk-full or transient volume error can therefore leave the manifest pointing at a missing or stale big file; a later hydrate may restore stale content.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
runpod/serverless/utils/rp_volume_cache.py:98
volume_pathcan be passed asbytes(sinceos.fspath()returns bytes unchanged). That will later raiseTypeErrorin_mirror_rootbecauseos.path.join()cannot mixbytesandstr. Sincedirsare normalized to text,volume_pathshould be fs-decoded to text as well.
self._volume_path = os.fspath(volume_path)
runpod/serverless/utils/rp_volume_cache.py:100
max_workers or defaulttreats0as “use the default” and allows negative / non-int values to flow intoThreadPoolExecutor, where they fail later with less clear errors. Validatemax_workersexplicitly and only apply the default when it isNone.
self._max_workers = max_workers or min(32, (os.cpu_count() or 4) * 4)
| _pending_syncs = [] | ||
|
|
||
|
|
||
| def _register_pending(thread): | ||
| _pending_syncs[:] = [t for t in _pending_syncs if t.is_alive()] | ||
| _pending_syncs.append(thread) | ||
|
|
||
|
|
||
| def _join_pending_syncs(): | ||
| for t in list(_pending_syncs): | ||
| try: | ||
| t.join(timeout=_JOIN_TIMEOUT_SECONDS) | ||
| if t.is_alive(): | ||
| log.warn( | ||
| "VolumeCache: background sync did not finish within " | ||
| f"{_JOIN_TIMEOUT_SECONDS:.0f}s at exit; cache may be incomplete" | ||
| ) | ||
| except Exception: | ||
| # best-effort shutdown: a join failure must not block interpreter exit | ||
| pass | ||
| _pending_syncs.clear() |
…t (SLS-367) _copy_parallel now returns (copied, current) source-path sets so callers can tell which files actually reached the volume. _do_sync records big_meta only for the copied|current union, so a big file whose copy failed is no longer written to the manifest as present -- preventing a later cold hydrate from serving stale volume bytes for it.
…ve-transport' into deanq/sls-367-volumecache-adaptive-transport
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
runpod/serverless/utils/rp_volume_cache.py:498
_pending_syncsis a shared mutable module-level list that may be mutated by multiple callers ofsync(background=True)concurrently and is also read/cleared by the atexit join. The current slice-assignment + append pattern is not thread-safe and can lose registrations in rare races. Guard_pending_syncswith athreading.Lockand snapshot threads to join outside the lock.
def _register_pending(thread):
_pending_syncs[:] = [t for t in _pending_syncs if t.is_alive()]
_pending_syncs.append(thread)
| def _tar_binary(self): | ||
| return shutil.which("tar") | ||
|
|
||
| def _pack_small(self, files): |
There was a problem hiding this comment.
if we write the tarfile here and then the manifest separately, is there a race condition where one worker could write a .tar and then have it get overwritten by another worker, but then the first worker still persists its manifest? it might not actually matter if the contents are the same anyway though
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
runpod/serverless/utils/rp_volume_cache.py:251
_copy_parallelcomputescurrentas “all sources not in todo”, but_needs_copy()returnsFalsewhen the source is missing/unstatable. That causes missing sources to be reported ascurrent, which contradicts the docstring (and could lead future callers to treat missing sources as mirrored). Filtercurrentto only include sources that still exist.
todo = [(s, d) for s, d in pairs if self._needs_copy(s, d)]
current = {s for s, _ in pairs} - {s for s, _ in todo}
if not todo:
return set(), current
Summary
Adds
VolumeCache, a serverless SDK primitive (SLS-367) that warms localmodel/cache directories from a mounted network volume and syncs deltas back,
persisting weights across worker recycling. Transport is size-bucketed and
chosen from an empirical benchmark on Runpod serverless (MooseFS network volume).
small.taron the volume (collapses per-file metadata round-trips).big/(incremental size/mtime diff; large-file subtree stays browsable).manifest.json, written last, is the atomic commit marker. Extraction always uses stdlibtarfilewith per-member path-traversal defense and per-member incremental skip.hydrate()/sync()/ context manager, plusmax_workers.Why
Benchmark across file-shape quadrants, mirror direction (local -> volume):
Serial per-file transport never wins and is catastrophic on many-small-file
caches: 72x slower than tar and 10x slower than parallel at 40k files. Large
files favor parallel (tar's single stream is dead weight). No single strategy
wins everywhere, so transport adapts to mean file size.
Design
tarbinary with atarfilefallback; if both are unavailable, small files reclassify to the unpacked big bucket (no hard failure).os.replace; the manifest rename is the linearization point (write-once/read-many cache lifecycle).best_effort=False. An unknown/corrupt/future-version manifest is treated as absent so sync self-heals.dirsentries are normalized to text (os.fsdecode) so abytespath can't makeos.walk/_is_safe_destraise aTypeErrorthat best-effort mode would swallow.Supersedes #531
This PR carries the complete SLS-367 deliverable: the VolumeCache primitive
(originally opened as #531 on the
deanquinanola/branch) plus the adaptivesize-bucketed transport, on the current
deanq/branch. #531 is closed infavor of this one. Base retargeted to
main.Linear: SLS-367
Test plan
make quality-check: 599 passing,rp_volume_cache.pycoverage 97% (repo floor 90%, total 94%).