Skip to content

Commit 54cbaee

Browse files
authored
Fix race condition in maybe_download (#846)
Fixes #845. `maybe_download` only checks if the file exists before acquiring the file lock, so multiple threads could pass the existence check concurrently, and then each will sequentially download the same file, resulting in redundant downloads. This PR adds an existence check after acquiring the file lock, so if multiple threads are concurrently downloading the same file, only one of them will actually perform the download and the others will shortcut. This is the code to reproduce: ```python from concurrent.futures import ThreadPoolExecutor import logging from openpi.shared.download import maybe_download logging.basicConfig(level=logging.INFO) def main(): n_workers = 2 with ThreadPoolExecutor(max_workers=n_workers) as executor: futures = [] for _ in range(n_workers): futures.append(executor.submit(maybe_download, "gs://openpi-assets/checkpoints/pi05_droid/params/ocdbt.process_0/d/b862fdd01b4477d01ea1f43ddb38b88f")) for future in futures: future.result() if __name__ == "__main__": main() ``` Before the fix, this was the output: ``` INFO:openpi.shared.download:Downloading gs://openpi-assets/checkpoints/pi05_droid/params/ocdbt.process_0/d/b862fdd01b4477d01ea1f43ddb38b88f to /root/.cache/openpi/openpi-assets/checkpoints/pi05_droid/params/ocdbt.process_0/d/b862fdd01b4477d01ea1f43ddb38b88f 100%|█████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 2.61G/2.61G [00:28<00:00, 99.9MiB/s] INFO:openpi.shared.download:Downloading gs://openpi-assets/checkpoints/pi05_droid/params/ocdbt.process_0/d/b862fdd01b4477d01ea1f43ddb38b88f to /root/.cache/openpi/openpi-assets/checkpoints/pi05_droid/params/ocdbt.process_0/d/b862fdd01b4477d01ea1f43ddb38b88f 100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 2.61G/2.61G [00:26<00:00, 108MiB/s] ``` With the fix, this is the output: ``` INFO:openpi.shared.download:Downloading gs://openpi-assets/checkpoints/pi05_droid/params/ocdbt.process_0/d/b862fdd01b4477d01ea1f43ddb38b88f to /root/.cache/openpi/openpi-assets/checkpoints/pi05_droid/params/ocdbt.process_0/d/b862fdd01b4477d01ea1f43ddb38b88f 100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 2.61G/2.61G [00:26<00:00, 108MiB/s] ```
2 parents 714ec9a + 78b4091 commit 54cbaee

1 file changed

Lines changed: 13 additions & 12 deletions

File tree

src/openpi/shared/download.py

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,18 +81,19 @@ def maybe_download(url: str, *, force_download: bool = False, **kwargs) -> pathl
8181
else:
8282
local_path.unlink()
8383

84-
# Download the data to a local cache.
85-
logger.info(f"Downloading {url} to {local_path}")
86-
scratch_path = local_path.with_suffix(".partial")
87-
# Route openpi-assets through gsutil to avoid gcsfs auth issues with this bucket.
88-
# All other gs:// URLs (e.g. big_vision) continue to use gcsfs as normal.
89-
if parsed.scheme == "gs" and parsed.netloc == "openpi-assets":
90-
_download_gsutil(url, scratch_path, **kwargs)
91-
else:
92-
_download_fsspec(url, scratch_path, **kwargs)
93-
94-
shutil.move(scratch_path, local_path)
95-
_ensure_permissions(local_path)
84+
if not local_path.exists():
85+
# Download the data to a local cache.
86+
logger.info(f"Downloading {url} to {local_path}")
87+
scratch_path = local_path.with_suffix(".partial")
88+
# Route openpi-assets through gsutil to avoid gcsfs auth issues with this bucket.
89+
# All other gs:// URLs (e.g. big_vision) continue to use gcsfs as normal.
90+
if parsed.scheme == "gs" and parsed.netloc == "openpi-assets":
91+
_download_gsutil(url, scratch_path, **kwargs)
92+
else:
93+
_download_fsspec(url, scratch_path, **kwargs)
94+
95+
shutil.move(scratch_path, local_path)
96+
_ensure_permissions(local_path)
9697

9798
except PermissionError as e:
9899
msg = (

0 commit comments

Comments
 (0)