Conversation
Split the memoized lookups so that a caller can supply an upper bound, and add a flag that a pool worker sets to report a single worker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The worker count for a subprocess is now set by a pool initializer, so it no longer depends on whether the parent had already calculated its own count and behaves the same under the fork and spawn start methods. Also stop dropping the caller's num_workers argument to mexists() when a process pool is in use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Schemes backed by a connection pool keep the modest default; file URIs hold no pool and so raise their bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each batch reports every URI independently, so one failure does not prevent the removal of the URIs that follow it in the same batch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Benchmarking local removal showed that extra threads add contention rather than throughput, because unlink on a local filesystem is not latency bound. The per-scheme bound remains available for a scheme that measures otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Under the spawn start method each worker pays a full interpreter startup, and the cost grows with the number of modules the calling process has imported. A pool is now kept alive and handed to later calls, and is discarded if it breaks. Thread pools are still created per call, since they are cheap and holding one open would keep its threads alive for no benefit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 83.46% 84.32% +0.85%
==========================================
Files 38 39 +1
Lines 8112 8492 +380
Branches 975 1000 +25
==========================================
+ Hits 6771 7161 +390
+ Misses 1044 1034 -10
Partials 297 297 ☔ View full report in Codecov by Harness. |
Existence checking now uses the same batching as removal, and the chunking helper is renamed to reflect that it serves both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timj
changed the title
timj
commented
Sep 15, 2026
|
|
||
|
|
||
| atexit.register(_clear_pool_executor_cache) | ||
| os.register_at_fork(after_in_child=_forget_pool_executor_cache) |
Member
Author
There was a problem hiding this comment.
If we keep the cache we probably want to consider making _clear_pool_executor_cache public to give us the option of having Butler.pruneDatasets() clear the pool cache so that we don't have the subprocesses hanging around. On the other hand the timing data suggest that there is no reason to use process instead of threads so happy to defer this. The cache won't really be used by butler.
Replace the cached pool when the executor class or worker count changes. Shut down the old pool so varying batch sizes do not accumulate workers. Add regression coverage for replacement and shutdown. Co-authored-by: Codex <noreply@openai.com>
Evict broken pools when bulk result handlers catch worker failures. Preserve per-resource failure reports while allowing the next caller to create a healthy pool. Cover existence, removal, transfer, and S3 removal. Co-authored-by: Codex <noreply@openai.com>
Register the cache cleanup hook only when os.register_at_fork exists. Test module loading without the hook and only exercise supported process start methods in worker tests. Co-authored-by: Codex <noreply@openai.com>
Three call sites reduced the worker count to the number of chunks a particular batch produced. With a single cached process pool that makes the pool's size a function of the batch size, so a small call replaces the pool built for a large one and the next large call has to build it again. Under the spawn start method each rebuild costs a full interpreter startup per worker. Asking for more workers than there is work for them costs nothing: ProcessPoolExecutor starts workers on demand for every start method except fork, and under fork a worker is cheap. Generated with AI Co-Authored-By: SLAC AI
Splitting a batch into one URI per chunk meant that checking three files
for existence occupied three workers, and under a process executor could
build a pool of subprocesses to do it. Handing the work over costs more
than the work itself at that size.
Chunks now have a per-scheme floor on their size, and a batch that
produces a single chunk is handled directly by the caller with no
executor involved. The floor is a scheme's property rather than a global
one because it expresses the cost of one operation: a missing-file check
on a local filesystem takes around 100 microseconds, so a batch has to
reach about a hundred URIs before spreading it wins, whereas a scheme
whose every operation is a network round trip is worth overlapping for
two. This generalizes the single-chunk shortcut that S3 bulk removal
already had.
Measured on wekafs with missing files, best of five, milliseconds:
threads process
N before after before after
1 0.37 0.08 20.38 0.07
3 0.54 0.22 20.60 0.25
30 3.14 2.12 23.75 2.28
300 9.56 10.03 31.48 36.31
1000 22.79 21.34 48.10 55.65
10000 270.81 213.09 266.96 259.08
Generated with AI
Co-Authored-By: SLAC AI
A site that sets AWS_REQUEST_CHECKSUM_CALCULATION to WHEN_REQUIRED stops boto3 computing a checksum on upload, so the object metadata carries no CRC32 and test_get_info fails on a developer machine while passing in CI. The fixture already removes the site's credentials and endpoint so that tests cannot reach real infrastructure; these two variables belong with them, because they change what the client does rather than where it points. Generated with AI Co-Authored-By: SLAC AI
timj
marked this pull request as ready for review
September 15, 2026 21:50
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.
On my Mac (so best case scenario with local disk) a simulated delete of a million files using 50k chunks (emulating butler usage) drops from 14 minutes per million to 2 minutes per million. Threads is still faster.
This is achieved mostly by using a cached process pool in subprocess mode and secondarily by passing multiple files to each worker in one call. Currently the cached worker pool hangs around forever -- we may want an API that butler can call to clean up once removeRuns or pruneDatasets has completed.
Checklist
doc/changes