Skip to content

Howie/poll timeout #40825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: feature/azure-ai-agents-v1
Choose a base branch
from
Open

Conversation

howieleung
Copy link
Member

@howieleung howieleung commented Apr 30, 2025

Add polling timeout

image

@Copilot Copilot AI review requested due to automatic review settings April 30, 2025 21:39
@github-actions github-actions bot added the AI label Apr 30, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces the initial implementation for the AgentsClient with both asynchronous and synchronous support. Key changes include the addition of a polling timeout configuration, updated client initialization logic, and new documentation for FunctionTool along with release notes.

Reviewed Changes

Copilot reviewed 155 out of 159 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
sdk/ai/azure-ai-agents/azure/ai/agents/aio/operations/init.py Initializes async operations and aggregates patch exports.
sdk/ai/azure-ai-agents/azure/ai/agents/aio/_configuration.py Adds polling_interval and configures async credential policies.
sdk/ai/azure-ai-agents/azure/ai/agents/aio/_client.py Sets up the async client; note potential issue with endpoint resolution.
sdk/ai/azure-ai-agents/azure/ai/agents/_configuration.py Updates sync configuration with polling and credential handling.
sdk/ai/azure-ai-agents/azure/ai/agents/_client.py Sets up the sync client; note potential issue with endpoint resolution.
sdk/ai/azure-ai-agents/azure/ai/agents/_types.py Defines several type unions, including a duplicate union member.
sdk/ai/azure-ai-agents/FunctionTool.md Provides FunctionTool specification documentation.
sdk/ai/azure-ai-agents/CHANGELOG.md Introduces initial release notes.
Files not reviewed (4)
  • pylintrc: Language not supported
  • sdk/ai/azure-ai-agents/MANIFEST.in: Language not supported
  • sdk/ai/azure-ai-agents/apiview-properties.json: Language not supported
  • sdk/ai/azure-ai-agents/assets.json: Language not supported
Comments suppressed due to low confidence (1)

sdk/ai/azure-ai-agents/azure/ai/agents/_types.py:15

  • [nitpick] The type definition includes a duplicate 'str' entry; consider removing the redundant type to improve clarity.
AgentsApiToolChoiceOption = Union[str, str, "_models.AgentsApiToolChoiceOptionMode", "_models.AgentsNamedToolChoice"]

def __init__(
self, endpoint: str, credential: Union[AzureKeyCredential, "AsyncTokenCredential"], **kwargs: Any
) -> None:
_endpoint = "{endpoint}"
Copy link
Preview

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _endpoint variable is set to a literal '{endpoint}' instead of using the actual endpoint parameter; consider using the provided endpoint value (for example, via an f-string) to ensure the client is initialized with the correct URL.

Suggested change
_endpoint = "{endpoint}"
_endpoint = endpoint

Copilot uses AI. Check for mistakes.

"""

def __init__(self, endpoint: str, credential: Union[AzureKeyCredential, "TokenCredential"], **kwargs: Any) -> None:
_endpoint = "{endpoint}"
Copy link
Preview

Copilot AI Apr 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to the async client, the sync client uses a literal '{endpoint}' for _endpoint rather than the actual endpoint value; update this to dynamically use the endpoint parameter.

Suggested change
_endpoint = "{endpoint}"
_endpoint = endpoint

Copilot uses AI. Check for mistakes.

@howieleung howieleung changed the base branch from main to feature/azure-ai-agents-v1 April 30, 2025 21:40
@jhakulin
Copy link
Member

jhakulin commented Apr 30, 2025

python-swe-agent: Main observations
──────────────────

  1. The coroutine does a blocking time.sleep inside an async def.
    • That freezes the whole event-loop while waiting and defeats the purpose of making the API asynchronous.
    • Use asyncio.sleep (or trio.sleep if you run under Trio) instead.

  2. Elapsed-time check is wrong and uses wall-clock time.
    time.time() can jump backwards/forwards if the system clock is adjusted.
    • You’re subtracting sleep_interval from the difference which gives an off-by-one iteration and makes reasoning harder.
    • Prefer time.monotonic() and compare directly against the start time.

  3. Polling loop does not break on “failed” / “cancelled” states and silently returns them.
    • Treat non-success terminal states explicitly and raise a descriptive error.

  4. The mutually exclusive parameter groups (body, file+purpose, file_path+purpose) are never enforced by type checkers and can produce ambiguous calls.
    • Validate that exactly one group is provided.
    • A dataclass/TypedDict or ParamUnion pattern would make this clearer to callers.

  5. HTTP-level timeout vs. polling timeout
    • The method uses timeout purely for polling. Callers may assume it is propagated to the underlying self.upload call. Consider exposing two names, e.g. request_timeout and polling_timeout.

  6. Miscellaneous
    • Prefer Literal for the status list (Python 3.11+ or typing_extensions).
    • Don’t leak **kwargs silently – forward them intentionally (**, request_kwargs).
    • Typos in error messages: method name is upload_and_poll, not upload_file_and_poll.
    • Docstring shows “:raises: HttpResponseError” (double colon).
    • Imports for time and asyncio are implicit.

import asyncio
import time
from typing import Any, Final, Optional, Union, overload

from azure.core.exceptions import HttpResponseError
from azure.ai.agents import models as _models
# from typing_extensions import Literal  # If < 3.11

JSON = Any
FileType = Any   # imported from sdk

_TERMINAL_SUCCESS:   Final = {"completed", "succeeded"}
_TERMINAL_FAILURE:   Final = {"cancelled", "failed", "error"}
_POLLABLE_STATES:    Final = {"uploaded", "pending", "running"}

class MyClient:
    # region: overloads --------------------------------------------------------
    @overload
    async def upload_and_poll(
        self, *, file: FileType, purpose: Union[str, _models.FilePurpose],
        filename: Optional[str] = None, sleep_interval: float = 1,
        polling_timeout: Optional[float] = None, **kwargs: Any
    ) -> _models.OpenAIFile: ...

    @overload
    async def upload_and_poll(
        self, *, file_path: str, purpose: Union[str, _models.FilePurpose],
        sleep_interval: float = 1, polling_timeout: Optional[float] = None,
        **kwargs: Any
    ) -> _models.OpenAIFile: ...
    # endregion

    # --------------------------------------------------------------------- #
    async def upload_and_poll(
        self,
        body: Optional[JSON] = None,
        *,
        file: Optional[FileType] = None,
        file_path: Optional[str] = None,
        purpose: Union[str, _models.FilePurpose, None] = None,
        filename: Optional[str] = None,
        sleep_interval: float = 1,
        polling_timeout: Optional[float] = None,
        **kwargs: Any,
    ) -> _models.OpenAIFile:
        """
        Convenience wrapper that uploads a file (or JSON body) and polls until
        it reaches a terminal state on the service.

        Raises
        ------
        ValueError
            If the combination of arguments is invalid.
        TimeoutError
            If the polling timeout elapses before the operation reaches a
            terminal state.
        HttpResponseError
            If the service returns an error when uploading or polling.
        """

        # --- choose the upload variant --------------------------------------
        if body is not None and (file or file_path):
            raise ValueError("`body` is mutually exclusive with `file` / `file_path`.")

        if body is not None:
            uploaded = await self.upload(body=body, **kwargs)                     # type: ignore[arg-type]
        elif file is not None and purpose is not None:
            uploaded = await self.upload(file=file, purpose=purpose,
                                         filename=filename, **kwargs)
        elif file_path is not None and purpose is not None:
            uploaded = await self.upload(file_path=file_path, purpose=purpose,
                                         **kwargs)
        else:
            raise ValueError(
                "Provide exactly one of: "
                "1) body, 2) file + purpose, 3) file_path + purpose."
            )

        # --- polling loop ----------------------------------------------------
        start = time.monotonic()
        while uploaded.status in _POLLABLE_STATES:

            # timeout?
            if polling_timeout is not None and time.monotonic() - start >= polling_timeout:
                raise TimeoutError(
                    f"Polling timed out after {polling_timeout} s "
                    f"(last status={uploaded.status!r})."
                )

            await asyncio.sleep(sleep_interval)
            uploaded = await self.get(uploaded.id)

        # --- final state -----------------------------------------------------
        if uploaded.status in _TERMINAL_FAILURE:
            raise HttpResponseError(
                message=f"File ended in failure state: {uploaded.status!r}",
                response=None,  # fill in if you have it
            )

        # success
        return uploaded

What changed & why
──────────────────
• Replaced time.sleepawait asyncio.sleep.
• Switched to time.monotonic() — immune to system-clock jumps.
• Removed the - sleep_interval offset: simpler and exact.
• Added explicit failure-state handling.
• Clearer parameter validation with mutually exclusive groups.
• Split polling_timeout from potential request-level timeouts.
• Forwarded **kwargs unchanged to upload; callers decide what to send.
• Fixed docstring/exception text inconsistencies.

These changes make the helper fully non-blocking, more robust to clock
changes, clearer to callers, and consistent with idiomatic async Python
and Azure SDK patterns.

@azure-sdk
Copy link
Collaborator

azure-sdk commented Apr 30, 2025

API Change Check

APIView identified API level changes in this PR and created the following API reviews

azure-ai-agents

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-ai-agents

@howieleung
Copy link
Member Author

6. Typos in error messages: method name is upload_and_poll, not upload_file_and_poll.

Did 1, 2, and Typos in error messages: method name is upload_and_poll, not upload_file_and_poll. within 6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants