-
Notifications
You must be signed in to change notification settings - Fork 16
Optimize tool call conversions to eliminate redundant API lookups #53
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
base: main
Are you sure you want to change the base?
Conversation
✅ Live Integration Testing CompleteAll 4 integration tests PASSED with live OCI GenAI API calls: Test Configuration:
Validation Results:
Performance Confirmed:
The optimization is production-ready and performs as expected with live API traffic. |
6cea04b to
72cda73
Compare
The test was failing after rebase because it used non-existent OCI SDK classes (models.Tool) and had incorrect expectations about when tool_choice is set to 'none'. Changes: 1. Replace OCI SDK mock objects with Python function (following pattern from other tests in the file) 2. Update test to trigger actual tool_choice=none behavior by exceeding max_sequential_tool_calls limit (3 tool calls) 3. Fix _prepare_request call signature (add stop parameter) 4. Pass bound model kwargs to _prepare_request (required for tools) 5. Update docstring to accurately describe what's being tested The test now correctly validates that tool_choice is set to ToolChoiceNone when the max_sequential_tool_calls limit is reached, preventing infinite tool calling loops. Related to PR oracle#50 (infinite loop fix) and PR oracle#53 (tool call optimization). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The test was failing after rebase because it used non-existent OCI SDK classes (models.Tool) and had incorrect expectations about when tool_choice is set to 'none'. Changes: 1. Replace OCI SDK mock objects with Python function (following pattern from other tests in the file) 2. Update test to trigger actual tool_choice=none behavior by exceeding max_sequential_tool_calls limit (3 tool calls) 3. Fix _prepare_request call signature (add stop parameter) 4. Pass bound model kwargs to _prepare_request (required for tools) 5. Update docstring to accurately describe what's being tested The test now correctly validates that tool_choice is set to ToolChoiceNone when the max_sequential_tool_calls limit is reached, preventing infinite tool calling loops. Related to PR oracle#50 (infinite loop fix) and PR oracle#53 (tool call optimization). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
72cda73 to
9cc8d4d
Compare
✅ Rebased on Latest MainSuccessfully rebased this PR on the latest Picked up commits from main:
Integration Tests: ✅ All 4 tests passing ✓ PASSED: Basic Tool Calling
✓ PASSED: Multiple Tools
✓ PASSED: Optimization Verification
✓ PASSED: Cohere ProviderThis PR is ready for review! 🎉 |
…#57) The test was failing after rebase because it used non-existent OCI SDK classes (models.Tool) and had incorrect expectations about when tool_choice is set to 'none'. Changes: 1. Replace OCI SDK mock objects with Python function (following pattern from other tests in the file) 2. Update test to trigger actual tool_choice=none behavior by exceeding max_sequential_tool_calls limit (3 tool calls) 3. Fix _prepare_request call signature (add stop parameter) 4. Pass bound model kwargs to _prepare_request (required for tools) 5. Update docstring to accurately describe what's being tested The test now correctly validates that tool_choice is set to ToolChoiceNone when the max_sequential_tool_calls limit is reached, preventing infinite tool calling loops. Related to PR #50 (infinite loop fix) and PR #53 (tool call optimization). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <[email protected]>
|
Hi @YouNeedCryDear! 👋 I hope all is well! I wanted to gently ping this PR that optimizes tool call processing to eliminate redundant API lookups. Why this matters: What's been validated:
Performance impact:
The change is purely internal optimization with no API changes, making it safe to deploy immediately. Would you have time to review this when convenient? I'm happy to address any concerns or questions! Really appreciate your stewardship of this project! 🙏 |
|
@fede-kamel There is a test folder, could you move the test file to the corresponding folder? Also integration test would only be a reference since it is not allowed to store secrets in Github per policy. It is better to separate those into small unit tests. |
Changes: - Moved test_tool_call_optimization.py to tests/integration_tests/chat_models/ - Renamed to test_tool_call_optimization_integration.py for clarity - Created new unit tests with mocked OCI client (tests/unit_tests/chat_models/test_tool_call_optimization.py) - Unit tests follow existing patterns with MagicMock and no OCI connection - Fixed integration test to use DEFAULT profile from environment variable Unit tests (enforced by CI): - test_meta_tool_call_optimization: Verifies Meta/Llama tool call caching - test_cohere_tool_call_optimization: Verifies Cohere tool call caching - test_multiple_tool_calls_optimization: Verifies multiple tool calls Integration tests (manual verification): - All 4 tests passing with live OCI API (Meta and Cohere) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
✅ Review Feedback AddressedThanks for the review @YouNeedCryDear! 1. ✅ Test File Moved to Correct FolderIntegration test relocated:
2. ✅ Unit Tests Created (Enforced by CI)Created mocked unit tests that CI will run automatically:
These unit tests will pass in CI without requiring OCI credentials. 🧪 Integration Test Results (Manual Verification)Tested with live OCI API: SummaryBoth review comments addressed:
The optimization is production-ready with both automated and manual test coverage! Commit: fede-kamel@2b44ca4 |
Tool call processing had significant redundancy: - chat_tool_calls(response) was called 3 times per request - Tool calls were formatted twice (once in chat_generation_info(), once in _generate()) - For requests with 3 tool calls: 9 total lookups instead of 3 (200% overhead) 1. Cache raw_tool_calls in _generate() to fetch once 2. Remove tool call formatting from Provider.chat_generation_info() methods 3. Centralize tool call conversion and formatting in _generate() 4. Add try/except for mock compatibility in hasattr checks - Before: 3 calls to chat_tool_calls() per request - After: 1 call to chat_tool_calls() per request - Reduction: 66% fewer API lookups for typical tool-calling workloads - No wasted UUID generation or JSON serialization All tool-related unit tests pass: - test_meta_tool_calling ✓ - test_cohere_tool_choice_validation ✓ - test_meta_tool_conversion ✓ - test_ai_message_tool_calls_direct_field ✓ - test_ai_message_tool_calls_additional_kwargs ✓ ✓ Same additional_kwargs format maintained ✓ Same tool_calls field structure preserved ✓ No breaking changes to public API ✓ All existing tests pass 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
- Created test_tool_call_optimization.py with 4 test cases - Tests basic tool calling, multiple tools, optimization verification, and Cohere provider - Added detailed PR_DESCRIPTION.md with: - Performance analysis and metrics - Code examples showing before/after - Complete unit test results - Integration test details and requirements - Backward compatibility guarantees
Changes: - Moved test_tool_call_optimization.py to tests/integration_tests/chat_models/ - Renamed to test_tool_call_optimization_integration.py for clarity - Created new unit tests with mocked OCI client (tests/unit_tests/chat_models/test_tool_call_optimization.py) - Unit tests follow existing patterns with MagicMock and no OCI connection - Fixed integration test to use DEFAULT profile from environment variable Unit tests (enforced by CI): - test_meta_tool_call_optimization: Verifies Meta/Llama tool call caching - test_cohere_tool_call_optimization: Verifies Cohere tool call caching - test_multiple_tool_calls_optimization: Verifies multiple tool calls Integration tests (manual verification): - All 4 tests passing with live OCI API (Meta and Cohere) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Fix convert_oci_tool_call_to_langchain to handle both Generic/Meta format (arguments as JSON string) and Cohere format (parameters as dict) - Fix attribute_map check to handle None values from mock objects - Fix line length issues in optimization code and tests - Update test mocks to use correct OCI SDK format
2b44ca4 to
24f31b6
Compare
✅ Rebased on Latest MainSuccessfully rebased this PR on the latest Changes in this rebase:
Test Results: @YouNeedCryDear - Ready for another look when you have a chance! All previous feedback has been addressed (tests moved to correct folders, unit tests created). Thanks! 🙏 |
Performance Optimization: Eliminate Redundant Tool Call Conversions
Overview
This PR optimizes tool call processing in
ChatOCIGenAIby eliminating redundant API lookups and conversions, reducing overhead by 66% for tool-calling workloads.Problem Analysis
Before Optimization
The tool call conversion pipeline had significant redundancy:
Impact:
Root Cause
The `format_response_tool_calls()` output went into `additional_kwargs` (metadata), but the actual `tool_calls` field used a different conversion path (`convert_oci_tool_call_to_langchain`). Both did similar work but neither reused the other's output.
Solution
1. Cache Raw Tool Calls in `_generate()`
2. Remove Redundant Formatting from Providers
3. Centralize Tool Call Processing
4. Improve Mock Compatibility
Performance Impact
Testing
Unit Tests (All Passing ✓)
```bash
$ .venv/bin/python -m pytest tests/unit_tests/chat_models/test_oci_generative_ai.py -k "tool" -v
tests/unit_tests/chat_models/test_oci_generative_ai.py::test_meta_tool_calling PASSED
tests/unit_tests/chat_models/test_oci_generative_ai.py::test_cohere_tool_choice_validation PASSED
tests/unit_tests/chat_models/test_oci_generative_ai.py::test_meta_tool_conversion PASSED
tests/unit_tests/chat_models/test_oci_generative_ai.py::test_ai_message_tool_calls_direct_field PASSED
tests/unit_tests/chat_models/test_oci_generative_ai.py::test_ai_message_tool_calls_additional_kwargs PASSED
================= 5 passed, 7 deselected, 7 warnings in 0.33s ==================
```
Test Coverage:
Integration Test Script
Created `test_tool_call_optimization.py` with 4 comprehensive test cases:
Test 1: Basic Tool Calling
Test 2: Multiple Tools
Test 3: Optimization Verification
Test 4: Cohere Provider
Note: Integration tests require OCI credentials. During development, we encountered expected 401 authentication errors when attempting live API calls. Recommendation: Oracle team should run `test_tool_call_optimization.py` with proper OCI credentials before merging to verify end-to-end functionality.
Backward Compatibility
✅ No Breaking Changes
✅ Code Structure
Files Changed
`libs/oci/langchain_oci/chat_models/oci_generative_ai.py` (+28, -16 lines)
`libs/oci/test_tool_call_optimization.py` (NEW, +300 lines)
Reviewers
This optimization affects the hot path for tool-calling workloads. Please verify:
Testing Checklist:
Deployment Notes: