fix: resolve conflicts for upstream sync PR #24 - #25
Closed
Cartofante wants to merge 105 commits into
Closed
Conversation
Redis Session Patch (COMPLETE) - Problem: Conversation context lost due to 10-second batch processing delay - Solution: Redis-based immediate session storage with graceful fallback - Status: Production-ready with comprehensive testing Ad discussed in BerriAI#12364
Fixes streaming ID inconsistency where streaming responses used raw provider IDs while non-streaming responses used properly encoded IDs with provider context. Changes: - Updated LiteLLMCompletionStreamingIterator to accept provider context - Added _encode_chunk_id() method using same logic as non-streaming responses - Modified chunk transformation to encode all streaming item_ids with resp_ prefix - Updated handlers to pass custom_llm_provider and litellm_metadata to streaming iterator Impact: - Streaming chunk IDs now format: resp_<base64_encoded_provider_context> - Enables session continuity when using streaming response IDs as previous_response_id - Allows provider detection and load balancing with streaming responses - Maintains backward compatibility with existing streaming functionality 🤖 Generated with [Claude Code](https://claude.ai/code)
…rmat Fixes streaming ID inconsistency where streaming responses used raw provider IDs while non-streaming responses used properly encoded IDs with provider context. Changes: - Updated LiteLLMCompletionStreamingIterator to accept provider context - Added _encode_chunk_id() method using same logic as non-streaming responses - Modified chunk transformation to encode all streaming item_ids with resp_ prefix - Updated handlers to pass custom_llm_provider and litellm_metadata to streaming iterator Impact: - Streaming chunk IDs now format: resp_<base64_encoded_provider_context> - Enables session continuity when using streaming response IDs as previous_response_id - Allows provider detection and load balancing with streaming responses - Maintains backward compatibility with existing streaming functionality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
This resolves MyPy type checking error where model_id could be None but wasn't explicitly typed as Optional[str].
Prevents 'Item None has no attribute get' error by checking for None before accessing litellm_metadata dictionary.
Adds unit and E2E tests to verify streaming chunk IDs are properly encoded with consistent format across streaming responses. ## Tests Added ### Unit Test (test_reasoning_content_transformation.py) - `test_streaming_chunk_id_encoding()`: Validates the `_encode_chunk_id()` method correctly encodes chunk IDs with `resp_` prefix and provider context ### E2E Tests (test_e2e_openai_responses_api.py) - `test_streaming_id_consistency_across_chunks()`: Tests that all streaming chunk IDs are properly encoded across multiple chunks in a real streaming response - `test_streaming_response_id_as_previous_response_id()`: Tests the core use case - using streaming response IDs for session continuity with `previous_response_id` ## Key Testing Approach - Uses **Gemini** (non-OpenAI model) to test the transformation logic rather than OpenAI passthrough, since the streaming ID consistency issue occurs when LiteLLM transforms responses rather than just passing through to native OpenAI responses API - Tests validate that streaming chunk IDs now use same encoding as non-streaming responses - Verifies session continuity works with streaming responses Addresses @ishaan-jaff's request for unit tests covering the streaming ID consistency fix. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Removes unused imports to fix CI linting errors: - GenericResponseOutputItem - OutputFunctionToolCall
Remove streaming ID consistency E2E tests as requested by @ishaan-jaff. Keep only the mock/unit test in test_reasoning_content_transformation.py
This reverts the streaming chunk ID encoding changes to understand the original issue better. Original behavior was: - Streaming chunks: raw provider IDs - Streaming final response: raw IDs (PROBLEM!) - Non-streaming final response: encoded IDs (correct) The real issue: streaming final response IDs were not encoded, breaking session continuity.
…ehavior Fixes streaming ID inconsistency to match OpenAI's Responses API behavior: - Streaming chunks: raw message IDs (like OpenAI's msg_xxx) - Final response: encoded IDs (like OpenAI's resp_xxx) This enables session continuity by ensuring streaming final response IDs have the same encoded format as non-streaming responses, allowing them to be used as previous_response_id in follow-up requests. Changes: - Add custom_llm_provider and litellm_metadata to LiteLLMCompletionStreamingIterator - Update handlers to pass provider context to streaming iterator - Apply _update_responses_api_response_id_with_model_id to final streaming response - Keep streaming chunks as raw IDs to match OpenAI format Impact: - Session continuity works with streaming responses - Load balancing can detect provider from streaming final response IDs - Format matches OpenAI's Responses API exactly 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Updates the unit test to verify streaming chunk IDs are raw (not encoded) to match OpenAI's responses API format: - Streaming chunks: raw message IDs (like msg_xxx) - Final response: encoded IDs (like resp_xxx) This reflects the correct behavior implemented in the fix.
- Add test_responses_api.py for testing multiple providers - Add responses_api_config.yaml with Claude, DeepSeek, and Gemini - Add RESPONSES_API_TEST_README.md with setup instructions - Tests session management with Redis for context retention - Validates basic responses, streaming, and session linking
The Response API wasn't storing sessions in Redis for streaming requests, only for non-streaming. This caused context to be lost when using previous_response_id with streaming responses. Changes: - Add _store_session_in_redis method to streaming iterator - Store full conversation history immediately when stream completes - Pass litellm_completion_request to streaming iterator for message history - Ensures streaming behaves identically to non-streaming for session storage This fixes the timing issue where a delay was needed between requests to allow batch processing to store sessions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…y' into chore/merge-streaming-id-consistency
Replace Python detection script with bash + gh CLI implementation.
Fix branch strategy to match CARTO requirements.
## Changes
**Workflow Updates:**
- Replace Python script with bash + gh CLI for detection
- Fix branch strategy:
1. BerriAI/litellm:main → CartoDB/litellm:main (auto-sync)
2. CartoDB/litellm:main → CartoDB/litellm:carto/main (PR)
- Add new job: sync-main-branch (auto-merges upstream/main)
- Update PR creation to target carto/main from main
- All logic now in pure bash (no Python dependencies)
**Files Removed:**
- .github/scripts/detect_stable_release.py
- .github/scripts/ directory (now empty)
**Documentation Updates:**
- CARTO_CLAUDE.md - Updated branch flow and detection method
- Removed Python script references
- Updated troubleshooting commands
## Key Improvements
- ✅ No Python dependencies (pure bash + gh CLI + git)
- ✅ Correct branch strategy (upstream → main → carto/main)
- ✅ Automatic main branch sync
- ✅ PR created with proper base/head branches
- ✅ Follows CARTO cloud-native patterns throughout
## Branch Strategy
```
BerriAI/litellm:main
↓ (auto-merge)
CartoDB/litellm:main
↓ (PR for review)
CartoDB/litellm:carto/main (production)
```
If the client sets the `labels` field in the request to the LiteLLM: - pass the `labels` field to the Vertex AI backend If the client sets the `metadata` field in the request to the LiteLLM: - if the `labels` field is not set, fill it with `metadata` key/value pairs for all string values
…14563) * Add comprehensive tests for Vertex AI Gemini labels provider filtering - Test Google GenAI endpoints exclude labels even when explicitly provided - Test Vertex AI endpoints include labels when provided - Cover provider detection logic for different endpoint URLs - Verify metadata-to-labels conversion only happens for Vertex AI - Ensure edge cases are handled properly (null/empty api_base) * Fix Vertex AI Gemini labels field provider-aware filtering - Add _is_google_genai_endpoint() function to detect Google GenAI vs Vertex AI endpoints - Update _transform_request_body() to accept api_base parameter - Only include labels field for Vertex AI endpoints (not Google GenAI) - Pass api_base through sync/async transform functions - Maintain backward compatibility with existing usage - Fixes issue where Google GenAI requests failed with unsupported labels field * Refactor labels filtering to use custom_llm_provider instead of URL parsing Replace URL-based endpoint detection with custom_llm_provider parameter checking for cleaner, more reliable provider identification. Changes: - Remove _is_google_genai_endpoint() helper function - Update labels condition to use custom_llm_provider != "gemini" - Remove api_base parameter from _transform_request_body() - Simplify sync/async transform function signatures - Update tests to reflect new parameter structure - Remove obsolete test_provider_detection test This approach aligns with existing codebase patterns where custom_llm_provider="gemini" identifies Google AI Studio endpoints that don't support labels, while vertex_ai/vertex_ai_beta identify Vertex AI endpoints that do support labels. * Use LlmProviders.GEMINI constant instead of hardcoded string
fix: 🐛 add Vertex AI labels support (cherry-picked from upstream)
- Add ::group:: and ::endgroup:: to all workflow steps for better log readability - Refactor Slack notification to use case statement (cleaner logic) - Switch to CARTO pattern: SLACK_KEY + channel ID instead of webhook - Update test-litellm.yml to trigger on PRs to carto/main (enables tests on sync PRs) - Extract workflow run URL to avoid repetition Follows CARTO cloud-native standards from deploy-dedicated-selfhosted action.
Add -X theirs merge strategy when syncing upstream/main to origin/main. Rationale: - main branch should be a pure mirror of upstream BerriAI/litellm - Any conflicts should be resolved in favor of upstream (theirs) - CARTO modifications belong in carto/main, not main - This prevents main from diverging from upstream Fixes merge conflicts like those in: - docker/Dockerfile.non_root - enterprise/litellm_enterprise/integrations/prometheus.py - litellm-proxy-extras migrations - litellm/constants.py - litellm/proxy/proxy_server.py - requirements.txt
refactor: replace Claude Code sync with bash-based workflow
…trategy Major changes: 1. Changed git config to use Cartofante (consistent with PAT user) 2. Added --no-edit flag to git merge for non-interactive execution 3. Refactored PR creation logic: - Now checks if carto/main is behind main (independent of upstream sync) - Creates PR whenever carto/main needs sync, not just on new releases - Renamed job: check-pr-exists → check-carto-main-sync 4. Updated Slack notifications to reflect new logic Benefits: - PR will be created even if main was already synced but carto/main is behind - More resilient: handles manual pushes to main - Better conflict resolution with --no-edit flag - Consistent git authorship (Cartofante) Workflow now performs two independent checks: 1. Is there a new upstream release? → Sync main 2. Is carto/main behind main? → Create PR (if no PR exists)
Changed from merge strategy to reset strategy for syncing main with upstream. Why: - git merge with -X theirs doesn't resolve ALL conflict types (rename/delete, add/add conflicts still fail) - main should be a PURE mirror of upstream, not a merge - reset --hard ensures main is EXACTLY like upstream/main Changes: - Replaced: git merge with git reset --hard upstream/main - Added: --allow-empty commit for history tracking - Changed: git push with --force-with-lease (safer than --force) This eliminates all merge conflicts since we're not merging, just replacing main's content with upstream's content.
Better approach following git best practices: Strategy: 1. Try git merge with -X theirs (git-friendly, preserves history) 2. If merge fails, abort and use git reset --hard (mirror fallback) 3. Intelligent push: only force when necessary Changes: - Added try-catch pattern for merge - Reset only used as fallback for unresolvable conflicts - Detects if force push is needed (checks fast-forward) - Documents in commit message which strategy was used - Stores previous HEAD reference when using reset Benefits: - Most syncs use normal merge (clean history) - Only force push when absolutely necessary - Better for git workflows and collaboration - Fallback ensures sync never fails This follows fork maintenance best practices while ensuring main always mirrors upstream.
Conflict resolution strategy: - Preserved CARTO customizations in infrastructure files (Dockerfile, requirements.txt) - Accepted upstream improvements to core LiteLLM functionality - Manually merged files with both CARTO and upstream changes Key resolutions: 1. docker/Dockerfile.non_root: - Kept CARTO's comprehensive Prisma caching setup - Kept CARTO's npm configuration and cache handling - Kept CARTO's OpenShift compatibility enhancements - Accepted upstream migration path handling 2. enterprise/litellm_enterprise/integrations/prometheus.py: - Accepted upstream fix: removed jitter parameter (memory leak fix) 3. requirements.txt: - Kept CARTO's nodejs-bin requirement for Prisma - Kept CARTO's litellm-proxy-extras disabled (built from source) - Accepted upstream dependency version updates (boto3, cryptography) 4. Migration files: - Accepted upstream versions (identical content) 5. Core LiteLLM files: - Accepted all upstream changes (constants, transformations, proxy, tests) Files with manual merge: - docker/Dockerfile.non_root (combined CARTO infra + upstream improvements) - requirements.txt (combined CARTO build process + upstream versions) This commit resolves all merge conflicts between upstream main and carto/main, preparing for the upstream sync PR #24 to be merged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔧 Automated Conflict Resolution
This PR resolves merge conflicts in #24 (upstream sync from BerriAI/litellm v1.78.5-stable).
🎯 Resolution Strategy
Followed strict priority order:
📋 Files Modified
CARTO versions kept:
docker/Dockerfile.non_root- Comprehensive Prisma caching, npm configuration, OpenShift compatibilityrequirements.txt- nodejs-bin requirement, litellm-proxy-extras disabled (built from source)Upstream versions accepted:
enterprise/litellm_enterprise/integrations/prometheus.py- Memory leak fix (removed jitter parameter)litellm/constants.py- Core constants updateslitellm/llms/vertex_ai/gemini/transformation.py- Vertex AI improvementslitellm/proxy/proxy_server.py- Proxy server enhancementslitellm/responses/litellm_completion_transformation/*- Response transformation improvementsManually merged:
docker/Dockerfile.non_root- Combined CARTO's infrastructure (Prisma caching, npm handling, OpenShift setup) with upstream migration improvementsrequirements.txt- Combined CARTO's build process (disabled litellm-proxy-extras, added nodejs-bin) with upstream dependency updates (boto3 1.36.0, cryptography 44.0.1)✅ Testing Results
Note: Full test suite not run due to environment constraints. The following validations were performed:
make lint- Deferred to CI/CDmake lint-mypy- Deferred to CI/CDmake test-unit- Deferred to CI/CD🔍 Review Guidelines
Please verify:
✅ CARTO Dockerfile customizations intact:
/tmp/.npm)✅ requirements.txt CARTO customizations intact:
nodejs-bin==18.4.0a4presentlitellm-proxy-extrascommented out with updated version reference (0.2.29)✅ Upstream bug fixes applied:
✅ Core LiteLLM functionality improved:
🚨 Important Changes
Upstream Bug Fix Applied:
enterprise/litellm_enterprise/integrations/prometheus.py:2192jitter=60parameter from scheduler.add_job()CARTO Infrastructure Preserved:
📝 Notes for Reviewers
Dockerfile changes: The resolved Dockerfile combines CARTO's infrastructure improvements (Prisma caching, npm handling) with upstream's migration path improvements. All CARTO customizations are preserved.
requirements.txt: Updated version reference in comment (0.2.29) while maintaining CARTO's local build approach.
Testing: Full test suite should be run by CI/CD. Manual verification recommended for:
docker build -f docker/Dockerfile.non_root ..github/workflows/carto_*.yaml)🤖 Automated conflict resolution by Claude Code
Resolves conflicts in #24