feat(py-client): implement "many" api for batch requests#546
feat(py-client): implement "many" api for batch requests#546matt-codecov wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #546 +/- ##
==========================================
- Coverage 87.22% 87.22% -0.01%
==========================================
Files 91 92 +1
Lines 14487 14843 +356
==========================================
+ Hits 12637 12947 +310
- Misses 1850 1896 +46
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Please see also the now closed #419. It's gone out of sync with main, so it's better to carry on here. The prior PR focused on streaming as much as possible and had gone through a round of feedback with the Python SDK maintainers for the public API. Also, there's configuration for concurrency that uses an optional thread pool executor. I hope there are some parts we could leverage from that. |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9ac7967. Configure here.
| try: | ||
| return self._send_batch(ops) | ||
| except Exception as exc: | ||
| return [_error_result_for(op, exc) for op in ops] |
There was a problem hiding this comment.
Batch parse error fails chunk
Medium Severity
When correlating a successful batch HTTP response, an exception while handling one multipart part (for example NotImplementedError from _maybe_decompress on a GET with non-zstd compression, or header parsing in Metadata.from_headers) aborts _send_batch. _run_batch then assigns that same exception to every operation in the chunk, so successful server-side ops are reported as failed. Individual many paths return one error per op instead.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9ac7967. Configure here.
| **not** guaranteed to be in the order the operations were enqueued. | ||
| """ | ||
| individual_concurrency = self._max_individual_concurrency or DEFAULT_INDIVIDUAL_CONCURRENCY | ||
| batch_concurrency = self._max_batch_concurrency or DEFAULT_BATCH_CONCURRENCY |
There was a problem hiding this comment.
Zero concurrency ignores setting
Low Severity
ManyBuilder.send() resolves concurrency limits with self._max_individual_concurrency or DEFAULT_INDIVIDUAL_CONCURRENCY (and the batch equivalent). A caller who sets either limit to 0 has that value treated as unset, so defaults (5 and 3) apply instead of a minimum of one worker, unlike the Rust client’s .max(1) clamp.
Reviewed by Cursor Bugbot for commit 9ac7967. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if key is None: | ||
| if is_error: | ||
| key = "<unknown>" |
There was a problem hiding this comment.
Bug: A successful keyless insert is incorrectly reported as an ErrorResult if the server returns a success status but omits the x-sn-batch-operation-key header.
Severity: MEDIUM
Suggested Fix
Make the client code more robust to server contract violations. For a successful response (2xx status) that is missing the x-sn-batch-operation-key header, consider returning a PutResult with a sentinel or None key instead of an ErrorResult. This would correctly represent the operation's success while indicating the key is unknown, preventing a successful write from being reported as a failure.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: clients/python/src/objectstore_client/many.py#L525-L527
Potential issue: The client-side parsing logic for batch operations assumes that a
successful response for a keyless insert will always contain the
`x-sn-batch-operation-key` header with the server-generated key. If the server returns a
success status (e.g., 2xx) but omits this header due to a bug, proxy issue, or future
change, the client code incorrectly interprets this as a failure. Instead of returning a
`PutResult`, it constructs and returns an `ErrorResult` with a "missing header" message.
This misrepresents a successful data insertion as an error to the caller, creating a
discrepancy between the client's state and the actual state on the server.
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
#277 / #478 implemented the
manyAPI in the Rust client which uses Objectstore's batch endpoint. This PR ports it over to Python.Closes FS-330