Repository navigation
Python: Preserve method tool invocation limits - #8951
Barry (barry166) wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
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
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.
b477fbb to
fb4c03e
Compare
fb4c03e to
4fabb35
Compare
|
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 |
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
|
Implemented in 556da45.
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
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.
|
Synced the branch with current Validation on the merged branch:
The PR is mergeable again. Public API compatibility and CLA are green; the remaining pull-request workflows still require maintainer action. |


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, somax_invocationsandmax_invocation_exceptionsare not enforced for the documented method-tool usage pattern. This fixes the reproduced behavior in #8904.Description & Review Guide
Related Issue
Fixes #8904
AI Assistance
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
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.