Skip to content

Python: Preserve method tool invocation limits - #8951

Open
Barry (barry166) wants to merge 4 commits into
microsoft:mainfrom
barry166:fix/method-tool-invocation-limits
Open

Barry (barry166) wants to merge 4 commits into
microsoft:mainfrom
barry166:fix/method-tool-invocation-limits

Conversation

@barry166

@barry166 Barry (barry166) commented Oct 1, 2026 •

Copy link
Copy Markdown

Motivation & Context

FunctionTool.__get__ creates a fresh shallow copy each time a decorated instance method is accessed. Invocation counters therefore reset when callers retrieve the method again, so max_invocations and max_invocation_exceptions are not enforced for the documented method-tool usage pattern. This fixes the reproduced behavior in #8904.

Description & Review Guide

  • What are the major changes? Cache bound method tools in each owner's instance dictionary, preserving copy-on-bind behavior and isolating equal or unhashable owners. Keep the original uncached binding behavior for slots-only owners without an instance dictionary.
  • What is the impact of these changes? Repeated access on owners with an instance dictionary shares invocation and exception counters. The owner-local cache can be garbage-collected, and pickle/deepcopy recreate it empty. Slots-only owners remain callable; callers must retain the returned tool to preserve its counters. Regression tests cover identity isolation, owner lifetime, copying, pickle round trips, and JSON serialization.
  • What do you want reviewers to focus on? Please verify owner lifetime, the documented slots-only fallback, and that serialization omits runtime bindings and restores an empty cache.

Related Issue

Fixes #8904

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: Codex assisted with review analysis, implementation, regression tests, validation, the PR description, and review responses. The contributor's personal-review acknowledgement is pending.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI balanced review requested due to automatic review settings October 1, 2026 16:15
@barry166
Barry (barry166) deployed to github-app-auth October 1, 2026 16:15 — with GitHub Actions Active
@barry166
Barry (barry166) deployed to github-app-auth October 1, 2026 16:15 — with GitHub Actions Active
@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Oct 1, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The cache leaks owner instances, breaks some valid owner types, aliases equal instances, and prevents pickle serialization.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Fixes method-tool invocation limits by caching bound FunctionTool instances per owner.

Changes:

  • Adds a weak-key bound-tool cache.
  • Adds regression tests for invocation and exception limits.
File Description
python/​packages/​core/​agent_framework/​_tools.py Caches bound method tools.
python/​packages/​core/​tests/​core/​test_tools.py Tests persistent method invocation counters.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py Outdated
@barry166
Barry (barry166) force-pushed the fix/method-tool-invocation-limits branch from b477fbb to fb4c03e Compare October 1, 2026 16:18
@barry166
Barry (barry166) deployed to github-app-auth October 1, 2026 16:19 — with GitHub Actions Active
@eavanvalkenburg

Copy link
Copy Markdown
Member

Barry (@barry166) The current owner cache assumes every valid owner is hashable and weak-referenceable, so unhashable instances and slotted objects without weakref support can fail during tool binding. The cache also remains in inherited pickle state even though it is excluded from dictionary serialization, so pickling a FunctionTool can still fail or retain runtime-only cache state. Please use an owner strategy that supports the existing valid object surface and explicitly omit/recreate the cache across pickle, with regressions for both cases.

Keep bound tools on their owner so unhashable and equal instances remain
isolated and the cache does not retain owners globally. Recreate runtime
bindings after pickle or copying, and preserve slots-only compatibility.

Tested: 590 related tests passed, 1 skipped; Ruff and Pyright passed
Constraint: Slots-only owners must retain the returned tool for persistent limits
@barry166
Barry (barry166) deployed to github-app-auth October 9, 2026 06:52 — with GitHub Actions Active
@barry166
Barry (barry166) deployed to github-app-auth October 9, 2026 06:53 — with GitHub Actions Active
@barry166

Barry (barry166) commented Oct 9, 2026 •

Copy link
Copy Markdown
Author

Implemented in 556da45.

  • Replaced the global weak-key cache with an owner-local cache. This supports unhashable and non-weak-referenceable owners with instance dictionaries, isolates equal owners, and removes the strong-reference leak.
  • Preserved uncached binding for slots-only owners without an instance dictionary. These callers must retain the returned tool for persistent invocation limits; the limitation is documented in the descriptor and PR description.
  • Runtime bindings are omitted during pickle/deepcopy and recreated on subsequent access. Added regressions for standalone/bound tool pickle, owner round trips, copying, lifetime, and JSON conversion.

Validation: 638 related tests passed, 1 skipped. Changed-file Ruff formatting/lint, strict source Pyright, and all five configured test typing checkers passed. Six new cases reproduce the original owner-compatibility/identity/lifetime failures when run against the previous descriptor implementation.

Synced with main in d2f3cae and reran the checks above. GitHub pull_request workflows currently require maintainer action before CI can run.

Tested: 638 related tests passed, 1 skipped; Ruff, source Pyright, and all five test typing checkers passed
@barry166
Barry (barry166) deployed to github-app-auth October 9, 2026 06:56 — with GitHub Actions Active
@barry166
Barry (barry166) deployed to github-app-auth October 9, 2026 06:57 — with GitHub Actions Active
Sync with current main and recreate the runtime-only invocation histogram after unpickling FunctionTool instances.

Tested: python core unit suite (8785 passed, 20 skipped, 2 xfailed); targeted test_tools.py (166 passed); Ruff, Pyright, ty, pyrefly, zuban, and mypy checks.
@barry166
Barry (barry166) deployed to github-app-auth October 20, 2026 05:32 — with GitHub Actions Active
@barry166

Copy link
Copy Markdown
Author

Synced the branch with current main in c04dffbf. The sync introduced a runtime-only OpenTelemetry histogram on FunctionTool; I excluded it from pickle state and recreate it on restore so the existing serialization regressions continue to pass.

Validation on the merged branch:

  • test_tools.py: 166 passed
  • core unit suite: 8785 passed, 20 skipped, 2 xfailed
  • Ruff formatting/lint and strict source Pyright
  • test typing checks: ty, pyrefly, zuban, Pyright, and mypy

The PR is mergeable again. Public API compatibility and CLA are green; the remaining pull-request workflows still require maintainer action.

This branch was successfully deployed

1 active deployment
github-app-auth — c04dffbf Deployed Oct 20, 2026 by barry166 via add_label #25795
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: max_invocations / max_invocation_exceptions are not enforced for tools defined as methods

3 participants