From 1b19b31edbd9d666cfbb738cc64648bdd269f0f0 Mon Sep 17 00:00:00 2001 From: Dean Rie Date: Thu, 22 Oct 2026 19:06:58 +0300 Subject: [PATCH] files: decide progress callback arity from its signature; total counts from the current position _invoke_progress tried progress(uploaded, total) and, on TypeError, fell back to progress(chunk_size). The except also caught TypeErrors raised inside the user's callback, so a two-argument callback that failed on its own (the docstring's own example does when total is None: 100 * uploaded / total) was silently called again with one argument and the user saw 'missing 1 required positional argument: total' instead of their error. Arity now comes from inspect.signature once per call; callables that cannot be inspected get the documented two-argument form. _chunk_file_from_fileobj (sync and async) computed total_bytes as the absolute end offset, so a file object positioned past a header reported a total the upload could never reach. It is now end minus current position, which is what gets uploaded. Tests: one-argument plain function gets chunk sizes; a TypeError from inside the callback propagates with its own message; a BytesIO read past a header reports (payload, payload) on the last call. Both new behaviours fail on main. --- CHANGELOG.md | 2 ++ src/xai_sdk/files.py | 39 ++++++++++++++++++---- tests/sync/files_test.py | 71 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bb7ce1..6893cba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # Changelog ## [Unreleased] +### Fixed +- `files.upload` progress callbacks: a two-argument callback that raises `TypeError` inside its own body is no longer mistaken for a one-argument callback and called again with the chunk size; the callback's arity is read from its signature once and user exceptions propagate unchanged. A file object positioned past the start now reports the bytes left to upload as `total`, not the file's full length. ## [v1.21.0](https://github.com/xai-org/xai-sdk-python/releases/tag/v1.21.0) - 2026-10-23 ### Added diff --git a/src/xai_sdk/files.py b/src/xai_sdk/files.py index cd51806..525c967 100644 --- a/src/xai_sdk/files.py +++ b/src/xai_sdk/files.py @@ -1,5 +1,6 @@ import asyncio import datetime +import inspect import os from typing import Any, AsyncIterator, BinaryIO, Callable, Iterator, Literal, Optional, Protocol, Union @@ -136,14 +137,38 @@ def _invoke_progress( progress.update(chunk_size) # type: ignore # Check if it's a callable (function) elif callable(progress): - # Try calling with both arguments first (custom callback with total) - try: + if _accepts_two_positional_args(progress): progress(cumulative_bytes, total_bytes) # type: ignore - except TypeError: - # If that fails, try calling with just incremental bytes (tqdm.update style) + else: + # Single-argument callables (e.g. tqdm.update) get the chunk size. progress(chunk_size) # type: ignore +# The documented progress callback shape is `(uploaded_bytes, total_bytes)`. +_PROGRESS_CALLBACK_ARGS = 2 + + +def _accepts_two_positional_args(func: Callable[..., Any]) -> bool: + """Whether `func` can be called with two positional arguments. + + Decided from the signature, so an exception raised inside the user's callback propagates + unchanged instead of being mistaken for an arity mismatch and retried with one argument. + Callables whose signature cannot be inspected (some builtins and C extensions) are called with + two arguments, matching the documented `(uploaded, total)` form. + """ + try: + signature = inspect.signature(func) + except (TypeError, ValueError): + return True + positional = 0 + for parameter in signature.parameters.values(): + if parameter.kind is inspect.Parameter.VAR_POSITIONAL: + return True + if parameter.kind in (inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD): + positional += 1 + return positional >= _PROGRESS_CALLBACK_ARGS + + def _order_to_pb(order: Order | None) -> files_pb2.Ordering: """Converts order string to protobuf enum.""" match order: @@ -273,7 +298,8 @@ def _chunk_file_from_fileobj( try: current_pos = file_obj.tell() file_obj.seek(0, 2) # Seek to end - total_bytes = file_obj.tell() + # Only the bytes from the current position onward are uploaded. + total_bytes = file_obj.tell() - current_pos file_obj.seek(current_pos) # Return to original position except (OSError, AttributeError): # Not all file objects support seek/tell (e.g., streams) @@ -415,7 +441,8 @@ async def _async_chunk_file_from_fileobj( try: current_pos = file_obj.tell() file_obj.seek(0, 2) # Seek to end - total_bytes = file_obj.tell() + # Only the bytes from the current position onward are uploaded. + total_bytes = file_obj.tell() - current_pos file_obj.seek(current_pos) # Return to original position except (OSError, AttributeError): # Not all file objects support seek/tell (e.g., streams) diff --git a/tests/sync/files_test.py b/tests/sync/files_test.py index 34ea4ea..a7ec27a 100644 --- a/tests/sync/files_test.py +++ b/tests/sync/files_test.py @@ -4,7 +4,7 @@ import io import os import tempfile -from typing import Callable, Iterable, Optional +from typing import BinaryIO, Callable, Iterable, Optional, cast from unittest import mock import pydantic @@ -218,6 +218,75 @@ def consume_chunks(chunks): os.unlink(temp_file_path) +def test_upload_progress_single_argument_function(client_with_mock_stub: Client, mock_stub): + """A one-argument function (tqdm.update style) receives the chunk size, decided from its signature.""" + data = b"X" * (4 * 1024 * 1024) + updates: list[int] = [] + + def update(n: int) -> None: + updates.append(n) + + mock_stub.UploadFile.side_effect = lambda chunks: (list(chunks), files_pb2.File(id="file-1"))[1] + + client_with_mock_stub.files.upload(data, filename="test.bin", on_progress=update) + + assert sum(updates) == len(data) + + +def test_upload_progress_callback_errors_propagate_unchanged(client_with_mock_stub: Client, mock_stub): + """A TypeError raised inside a two-argument callback is the user's error, not an arity hint. + + Before, it was swallowed and the callback was called again with one argument, surfacing a + misleading "missing 1 required positional argument" instead of the real failure. + """ + + seen: list[str] = [] + + def progress(uploaded: int, total: Optional[int]) -> None: + seen.append(f"{100 * uploaded / total:.1f}%") # type: ignore[operator] # fails when total is None + + mock_stub.UploadFile.side_effect = lambda chunks: (list(chunks), files_pb2.File(id="file-1"))[1] + + class Unsized(io.RawIOBase): + """Readable stream without seek/tell, so the total is unknown.""" + + def __init__(self, payload: bytes): + self._buffer = io.BytesIO(payload) + + def readable(self) -> bool: + return True + + def read(self, size: int = -1) -> bytes: + return self._buffer.read(size) + + def seek(self, _offset: int, _whence: int = 0) -> int: + raise OSError("not seekable") + + def tell(self) -> int: + raise OSError("not seekable") + + stream = cast(BinaryIO, Unsized(b"abc")) + with pytest.raises(TypeError, match="unsupported operand|NoneType"): + client_with_mock_stub.files.upload(stream, filename="stream.bin", on_progress=progress) + + +def test_upload_progress_total_counts_from_current_position(client_with_mock_stub: Client, mock_stub): + """A file object positioned past a header reports the bytes it will upload as the total.""" + header = b"HEADER" + payload = b"Y" * 1024 + file_obj = io.BytesIO(header + payload) + file_obj.read(len(header)) + calls: list[tuple[int, Optional[int]]] = [] + + mock_stub.UploadFile.side_effect = lambda chunks: (list(chunks), files_pb2.File(id="file-1"))[1] + + client_with_mock_stub.files.upload( + file_obj, filename="rest.bin", on_progress=lambda uploaded, total: calls.append((uploaded, total)) + ) + + assert calls[-1] == (len(payload), len(payload)) + + def test_upload_with_progress_tqdm_like(client_with_mock_stub: Client, mock_stub): """Test uploading a file with tqdm-like progress object.""" data = b"X" * (5 * 1024 * 1024) # 5MB