enh: replace local input file copy with S3 ranged gets - #554
Conversation
|
Unsigned commits detected! Please sign your commits. For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation. |
9ac5299 to
5cde401
Compare
| defer file.Close() | ||
|
|
||
| buf := make([]byte, length) | ||
| if _, err := file.ReadAt(buf, offset); err != nil { |
There was a problem hiding this comment.
there miight be an issue
func readNormalizedLine in preprocessor.go in will add \n, so the file length is N, but plan is N+1
then file.ReadAt(buf, N+1) will return EOF error?
There was a problem hiding this comment.
I think you're right, thank you! Will fix this.
|
Thanks for looking into disk usage optimization, Antonio. Before we proceed with the review, I want to share some concerns about the approach and suggest a different workflow. Process suggestion: For changes of this scope (18 files, new interface method, rearchitecting the executor's I/O path), it would be better to start with an issue describing the problem and proposed design. That gives us a chance to discuss trade-offs and agree on an approach before investing in implementation. Happy to collaborate on the design there. On the problem statement: The PR describes a worst-case of 20 workers × 200MB = 4GB of
The problem is real in theory at extreme scale, but it's not clear this is hitting anyone today. On the proposed solution: Replacing local
Simpler alternatives worth discussing: If disk pressure does become a real concern at high worker counts, there are lighter-weight options:
I'd suggest we close this PR, open an issue with the problem statement and these alternatives, and converge on a design before writing code. What do you think? |
NumWorkers defaults to 1 in the Go config code (https://github.com/llm-d/llm-d-batch-gateway/blob/cc34fa9/internal/processor/config/config.go#L300), but both the Helm chart (https://github.com/llm-d/llm-d-batch-gateway/blob/cc34fa9/charts/batch-gateway/values.yaml#L307) and the shipped config file (https://github.com/llm-d/llm-d-batch-gateway/blob/cc34fa9/cmd/batch-processor/config.yaml#L4) override it to 20, which is the operational default that all deployments use. So the 20 × 200 MB = 4 GB calculation applies to any default deployment with max-size files. The emptyDir sizeLimit defaults to 10Gi (https://github.com/llm-d/llm-d-batch-gateway/blob/cc34fa9/charts/batch-gateway/values.yaml#L237), and input copies alone can consume 40% of that budget before accounting for output/error files growing during execution.
Per-request latency overhead of S3 range GETs (5–50ms) is irrelevant for the batch gateway. Inference latency is hundreds of ms to seconds — the S3 overhead is noise. This is a batch system, not a real-time serving path.
S3 is already a required dependency. A network blip during execution that prevents S3 range reads would equally prevent finalization from uploading results. The failure mode isn't new, it's the same dependency the system already has. Additionally, the retryclient wrapper handles transient errors with exponential backoff.
100 concurrent S3 GET requests is well within S3's capabilities. AWS documents 5,500 GET requests per second per prefix (https://docs.aws.amazon.com/AmazonS3/latest/userguide/optimizing-performance.html), and S3-compatible stores (MinIO, Ceph) handle similar throughput. 100 concurrent requests is not a concern.
The multiple-context pattern is already used extensively throughout the executor. On main, there are 10 functions that take 2+ context parameters:
This is already set (10Gi default (https://github.com/llm-d/llm-d-batch-gateway/blob/cc34fa9/charts/batch-gateway/values.yaml#L237)). It doesn't solve the problem, it just makes the pod get evicted when it exceeds the limit. The point is to not need the disk space at all.
This moves the same 200 MB × 20 workers = 4 GB problem from disk to RAM, which is worse, RAM is more expensive, and OOM kills are harder to recover from than disk pressure.
Still requires the file on disk. Doesn't reduce disk usage at all, it just changes how the kernel pages it.
Adds throttling complexity to work around a resource problem that this PR eliminates entirely. |
|
+1 on the direction. I view this type of change necessary as a crash safety improvement, not just a disk usage optimization. Running out of emptyDir can cause pod eviction and takes out every in-flight job on that worker. Because startup recovery is workdir-based, eviction destroys the partial outputs too, so those jobs land as failed with nothing salvageable whenever the orphan reconciler notices. Dropping the biggest unbounded consumer of that disk budget, and reading from the source of truth instead of a local copy, is actually less moving parts from a correctness PoV. This is also a step on the rung to more stateless workers in general. The less we can use temporary scratch space the better. |
| if err := json.Unmarshal(bytes.TrimSuffix(buf[:entry.Length], []byte{'\n'}), &req); err == nil { | ||
| customID = req.CustomID | ||
| } | ||
| req, _, _, readErr := p.readRequestLine(storageCtx, entry, inputRef, logger) |
There was a problem hiding this comment.
If entries are known apriori contiguous here, we can batch them together and do one combined read as an optimization during the drain, correct?
There was a problem hiding this comment.
Yes, very good point!
There was a problem hiding this comment.
Looking at this more I think we might be able to skip this read entirely if we start storing the custom_id in the plan entry and make it available here, since it's pre-calculated and then thrown away
There was a problem hiding this comment.
An issue is that the the plan entry is a fixed 16 bytes binary record, adding custom_id would break the fixed-size invariant. Honestly the plan files could be just moved to memory and custom_id be added. Maybe as a follow up?
There was a problem hiding this comment.
and tbh we can do batched reads anyway (with a fixed-size in-memory buffer) for multiple entries even in the normal path of a job execution.
|
This discussion is a good example of why design issues before PRs matter — we're iterating on fundamental trade-offs in PR comments, which is harder to follow and means implementation work may need to be redone. The PR does one S3 range GET per request in the batch. For There can be options to reduce the S3 calls volume by chunking and using buffers, e.g. for contiguous ranges or per model. |
100 sustained concurrent s3 calls is impossible in practice, no? There is a global rate limiter that affects the entire request lifecycle which is high hundreds of milliseconds to seconds of latency. The inference throughput would need to be several orders of magnitude faster to cause that level of S3 read contention
I think these optimizations are usually non-obvious without some initial implementation/PoC work, up front design is usually not the best place to hash those specifics out, experimentation and benchmarking will be required. |
I agree, however these types of large refactoring projects that have high level trade-offs and options, should be documented and discussed first in an organized proposal, rather than immediate PR. |
I can create a github issue but I don't think this deserves a design document or anything like that. I think this PR solves an obvious issue. I've already answered point by point in #554 (comment). Also PRs about implementation specific things like these is usually where discussions happen. |
| buf := make([]byte, entry.Length) | ||
| if _, err := inputFile.ReadAt(buf, entry.Offset); err != nil { | ||
| func (p *Processor) readRequestLine(ctx context.Context, entry planEntry, inputRef *inputFileRef, logger logr.Logger) (*batch_types.Request, string, *outputLine, error) { | ||
| rc, err := p.files.storage.RetrieveRange(ctx, inputRef.storageName, inputRef.folderName, entry.Offset, int64(entry.Length)) |
There was a problem hiding this comment.
Performance: every call to readRequestLine issues a separate S3 ranged GET. This affects all code paths — the main async dispatch loop (processModelAsync), the sync path (processModel/executeOneRequest), and the drain path (drainUnprocessedRequests).
For a 100K-request batch, that's 100K sequential S3 round-trips in the normal execution path alone, plus potentially thousands more during drain on cancellation/expiry.
This should use batched/chunked reads — fetch a block of consecutive entries in one ranged GET into a buffer, then serve individual readRequestLine calls from that buffer. The old code's local ReadAt was effectively free; the S3 replacement needs to amortize network overhead.
There was a problem hiding this comment.
How are you getting these numbers?
Right now the code has semaphores which bound the total number of concurrent inference requests. There is a 100 cap on the number of requests that can happen in parallel at any given time across all workers, this means there can only be 100 S3 ranged Gets at any given time, this is far more than acceptable for me. Arguably we could push it further without issues.
https://github.com/llm-d/llm-d-batch-gateway/blob/cc34fa9/charts/batch-gateway/values.yaml#L313
https://github.com/llm-d/llm-d-batch-gateway/blob/cc34fa9/internal/processor/worker/executor.go#L466
Also the goroutine making the inference request will spend only a handful of milliseconds on the S3 Gets, the actual inference request will take multiple seconds in the best case making this time (compared to doing FS reads) really negligible.
In theory there can be only 100 Gets in parallel, in practice I expect there's going to be ~10 at most at any time given that these are really quick compared to everything else the job's goroutine have to do to complete.
One more thing, the async path is completely sequential in that regard, there's 1 S3 get per worker at any given time, so even less of an issue.
There was a problem hiding this comment.
@acardace I agree the overhead is currently small relative to inference time. My concern is about the design pattern itself.
Per-line network I/O is a universally avoided pattern, even when each individual call is fast. Read-ahead is universally used when sequential access is known, not only because of the per-line-read cost, but also because per-line-read cost is unnecessary. The reason is that network round-trips carry fixed overhead (HTTP request/response framing, server-side request processing, S3 API call billing) that doesn't exist with local file I/O. The previous ReadAt on a local file got OS page-cache buffering for free — sequential reads were effectively memory reads. S3 has no equivalent; each ranged GET is a full HTTP request regardless of how small the payload is.
Also, "inference dominates" holds today but isn't guaranteed in the future — faster / smaller models, better hardware, or larger batch sizes can modify the ratio. I/O buffering removes the dependency on that assumption.
The fix is also not complex — fetching chunks of consecutive entries in a single ranged GET into a local buffer, then serving individual readRequestLine calls from that buffer. This brings the I/O cost profile back in line with the local-file behavior the code replaced.
This is about not introducing an unnecessary per-record network I/O pattern when a standard buffering approach avoids it.
There was a problem hiding this comment.
@acardace I agree the overhead is currently small relative to inference time. My concern is about the design pattern itself.
Per-line network I/O is a universally avoided pattern, even when each individual call is fast. Read-ahead is universally used when sequential access is known, not only because of the per-line-read cost, but also because per-line-read cost is unnecessary. The reason is that network round-trips carry fixed overhead (HTTP request/response framing, server-side request processing, S3 API call billing) that doesn't exist with local file I/O. The previous
ReadAton a local file got OS page-cache buffering for free — sequential reads were effectively memory reads. S3 has no equivalent; each ranged GET is a full HTTP request regardless of how small the payload is.
You can't do read-ahead right now with the current code design, entries are sorted by prefix-hash not by offset. I think we have to argue with what we have right now, this is a batch system, latency doesn't matter that much here, disruption of live requests and throughput does.
This change betters the crash resiliency story so IMHO it's much better than having huge fs consumption, especially when we're going to crank up the limits allowing more parallel jobs to be handled.
Read-ahead can be implemented in the future with a bigger design refactor.
Also, "inference dominates" holds today but isn't guaranteed in the future — faster / smaller models, better hardware, or larger batch sizes can modify the ratio. I/O buffering removes the dependency on that assumption.
Again, let's deal with what we have right now, I'm not sure we'll quickly see (if ever) see LLM inference that takes ~10ms so that an S3 Get becomes the bottleneck. We're really arguing about something that likely takes 1% of the time slot of a request.
The fix is also not complex — fetching chunks of consecutive entries in a single ranged GET into a local buffer, then serving individual
readRequestLinecalls from that buffer. This brings the I/O cost profile back in line with the local-file behavior the code replaced.This is about not introducing an unnecessary per-record network I/O pattern when a standard buffering approach avoids it.
Again I think doing better in regards to crash resiliency and recovery is more important, using S3 as the single source of truth can help us resume jobs much more easily as well.
a9a32e4 to
398f1d0
Compare
08d95f6 to
b917fc6
Compare
b917fc6 to
7e41e0b
Compare
Add RetrieveRange(ctx, fileName, folderName, offset, length) to the BatchFilesClient interface for fetching byte ranges from stored files. Implementations: - S3: uses GetObjectInput.Range header (bytes=start-end) - FS: opens file, ReadAt into buffer, returns io.NopCloser - retryclient: wraps with retry logic (fresh GET per attempt) - tracing: adds OTel span with offset/length attributes - mock: file-backed ReadAt for testing This enables the executor to read individual request lines on demand from shared storage instead of requiring a full local copy. Signed-off-by: Antonio Cardace <acardace@redhat.com>
Remove the local input.jsonl copy from the preprocessor and executor. The preprocessor still streams the full file for validation and plan building, but no longer writes a local copy. The executor fetches each request line on demand from shared storage via RetrieveRange. This eliminates per-job emptyDir usage for input data (up to 200 MB per job), leaving only output.jsonl and error.jsonl on local disk. Signed-off-by: Antonio Cardace <acardace@redhat.com>
Adapt all worker tests to use inputFileRef instead of *os.File. setupExecutionJob and setupAsyncExecutionJob now seed the mock files client and FileDB so resolveInputFileCoords works. Add testInputFileRef helper for tests calling executeOneRequest/processModel directly. Remove local input.jsonl assertions from preprocessor tests. Signed-off-by: Antonio Cardace <acardace@redhat.com>
7e41e0b to
b01fe15
Compare
|
I took a close look at the behavior changes here. The plan-file path still builds plans by streaming the input once, so removing the local input copy by itself looks fine and I did not find a separate correctness issue there. However, I do think the inline issue around RetrieveRange(context.Background()) should be fixed before this lands, because it changes the cancellation / SLO expiry / SIGTERM behavior in a worse direction than the old local ReadAt path. I also share Lior's concern that moving per-request input access from local random reads to remote ranged reads introduces a potentially expensive hot-path trade-off. I understand the argument that this PR is primarily about reducing local disk pressure / crash risk, and I’m okay treating the performance side as follow-up work, but I do think that concern should be explicitly validated, either in this PR or in the follow-up issue. |
| func readRequestLine(inputFile *os.File, entry planEntry, logger logr.Logger) (*batch_types.Request, string, *outputLine, error) { | ||
| buf := make([]byte, entry.Length) | ||
| if _, err := inputFile.ReadAt(buf, entry.Offset); err != nil { | ||
| func (p *Processor) readRequestLine(entry planEntry, inputRef *inputFileRef, logger logr.Logger) (*batch_types.Request, string, *outputLine, error) { |
There was a problem hiding this comment.
RetrieveRange is now remote I/O, but this is using context.Background() instead of a request-scoped context. That means cancel / SLO expiry / SIGTERM cannot interrupt a slow or stuck range read, so dispatched goroutines can stay blocked here holding semaphores and delay the errCancelled / errExpired / errShutdown paths. Before this change the input access was local ReadAt, so this can be a behavior regression rather than just an optimization concern.
There was a problem hiding this comment.
That's a fair point, what about using the abortCtx to detect cancellation and have the worker go through draining in case there's a network error? Would that be ok?
There was a problem hiding this comment.
or a simple
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
rc, err := p.files.storage.RetrieveRange(ctx, inputRef.storageName, inputRef.folderName, entry.Offset, int64(entry.Length))
might be enough?
There was a problem hiding this comment.
requestAbortCtx so cancel / expiry / shutdown can interrupt a slow read.
I don't agree with this, as I said before this is not a component delivering real-time requests, the batch gateway most important metrics are throughput and the ability to not disrupt live traffic, having ranged S3 gets before dispatching an inference request doesn't degrade these key metrics, and it improves the resiliency of the component. Also the time taken by an S3 request is orders of magnitude smaller than the inference request (we're talking about ~10-50ms vs seconds or even minutes) so I don't think the performance argument stands here, if this would be handling live traffic then it would be a different story altogether. I honestly don't think performance is impacted at all by this patch. |
That makes sense, and I agree this is not really a single-request latency concern. I’m more worried about the cost / request-volume trade-off of moving from one remote fetch plus local |
|
This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the |
Summary
The preprocessor streams the input file from S3 and writes a full copy to local
disk so the executor can do random-access reads by byte offset. With 20 workers
at the OpenAI max of 200 MB per file, this consumes up to 4 GB of
emptyDirforinput copies alone, plus the output/error files growing during execution.
This PR adds a
RetrieveRangemethod to theBatchFilesClientinterface thatfetches a byte range from storage (using the S3
Rangeheader), and replaces alllocal input file reads in the executor with ranged storage reads.
Changes
Interface & implementations
RetrieveRange(ctx, fileName, folderName, offset, length)toBatchFilesClientPreprocessor
input.jsonlwrite — the preprocessor still streams the full filefor validation and plan building, but no longer writes a local copy
which match the remote file layout
Executor
inputFileRefstruct holding remote storage coordinates (storageName,folderName)resolveInputFileCoords— resolves aninputFileIDto storage coordinates via DB lookup (once per job)readRequestLinefrom package-level function toProcessormethod that callsRetrieveRangeinternally*os.Filewith*inputFileRefthroughoutexecuteJob,processModel,processModelAsync,executeOneRequest,drainAndFinalize,drainUnprocessedRequestsdrainUnprocessedRequeststakes a separatestorageCtx(mainCtx) for S3 reads sinceprogressCtx(requestAbortCtx) may be cancelled during drainRemoved
createLocalInputFile,jobInputFilePath,inputFileNameconstantWhat stays on local disk
output.jsonlanderror.jsonl(could be streamed to S3 as a follow-up)Testing
TestRetrieveRangetests for S3, FS, and retryclient implementationsFixes #561