Repository navigation
Parallelize typed object uploads and harden mutations - #398
sergiopeixoto-seequent wants to merge 1 commit into
Conversation
| upload = ObjectDataUpload(connector=self._connector, environment=self._environment, name=table_info["data"]) | ||
| async for attempt in retry: | ||
| try: | ||
| await upload.upload_from_cache(cache=self._cache, transport=self._connector.transport, fb=fb) |
There was a problem hiding this comment.
You might want to check if the uploads are being rate limited as much as you wanted, because it looks like each upload_from_cache() allows for ``min(32, os.cpu_count() + 4)` workers.
There was a problem hiding this comment.
The semaphore limits eight complete table pipelines, including Parquet writing, but it does not impose an eight-chunk limit. Each active table inherits the common uploader’s min(32, os.cpu_count() + 4) chunk limit. With sufficiently large files, that can allow up to 8 × 32 = 256 chunk transfers to be reading data or awaiting transfer.
I also checked the transport: the default shared AioTransport limits actual HTTP connections to four, so those 256 chunk slots do not mean 256 simultaneous HTTP requests. However, that connection limit is configurable, and chunks can already be buffered while waiting for a connection. It therefore doesn’t eliminate the aggregate memory/concurrency concern.
The intended limit in this PR is eight table pipelines, not eight storage requests. Retry backoff handles transient failures but does not establish a shared chunk budget. I would keep the existing chunk behavior for that documented scope; if we want a strict overall storage-transfer limit, we need to implement that explicitly rather than rely on CPU count or retries.
| return table_info | ||
| if retry is None: | ||
| retry = Retry(logger=logger) | ||
| async with get_upload_semaphore() or self._upload_semaphore: |
There was a problem hiding this comment.
I thought it was a bit confusing to have two semaphores. (It's not a request for change - I just had a quick look out of interest and left my 2c)
There was a problem hiding this comment.
They’re alternatives, not two locks acquired together. get_data_client() creates separate client instances, so a per-client semaphore alone would let each client start eight uploads independently.
The context semaphore keeps all nested serialization under one shared eight-table limit.
Outside that serialization scope, the client semaphore bounds directly concurrent upload_table() calls. The or selects which scope applies.
|
This is a valuable improvement for all compute tasks that upload large data. We observed multi-minute client-side upload spans in conditional simulation, while the corresponding bulk-upload burst in the logs completed in roughly 34 seconds. Investigating the current SDK showed synchronous Parquet/gzip serialization and hashing inside My comments fwiw: ❓ Could we make the Out of the scope of this PR, but since I suppose both our teams are looking for the same outcome: We should reuse data clients per workspace within the task’s |
425e92b to
152dda6
Compare
Restore the original upload changes on current main, retaining upstream error context and covering retry exhaustion diagnostics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Typed-object creation and replacement uploaded independent tables, attributes, and collections sequentially, making large uploads slow. Some mutations also changed the in-memory object before every upload had succeeded, risking partially updated state on failure.
The branch parallelizes independent uploads with a shared concurrency limit, preserves input order, retries transient upload failures, and commits mutations only after all required uploads succeed. It adds focused tests for concurrency, retries, cancellation, ordering, and mutation atomicity.
Checklist