Fix: Incorrect property access causing indexing failures #200
Conversation
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
The mise-action was hanging during the "Exporting mise environment variables" step, causing the runner to kill the job with exit code 137 (SIGKILL). This was happening because: 1. The mise.toml has complex template expressions with command executions 2. The hooks.enter configuration may execute during env export 3. The env export was timing out after ~1 minute Solution: Disable the env parameter (set to false) in all mise-action configurations to prevent the hanging during environment variable export. Fixes #issue
…ZvD3Lo3vX7tdaw9xrKC' into fix-test-ci-action-failures
The previous fix disabled env export to prevent hanging, but this caused MISE_PATH to be empty because mise wasn't in the PATH. Changes: 1. Add mise bin directory to PATH in "Set outputs" step 2. Add mise bin directory to PATH in workflow steps that execute mise commands This ensures: - The "Set outputs" step can find mise using which - Workflow steps can execute mise commands via $MISE_PATH - No hanging during env export (env: false is still set) Fixes the "command not found" error when running mise commands.
…ZvD3Lo3vX7tdaw9xrKC' into fix-test-ci-action-failures
The composite action was setting outputs in the mise-outputs step, but those outputs were not being exposed by the action itself because the action.yml was missing the outputs section. Changes: - Add outputs section to action.yml defining all output variables - Map outputs from mise-outputs step to action outputs This ensures that steps.setup-mise.outputs.MISE_PATH and other outputs are properly available to the calling workflow. Related to the CI skip issue fix.
…ZvD3Lo3vX7tdaw9xrKC' into fix-test-ci-action-failures
Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]>
Remove the first instance of "Setup Mise Environment" step (lines 52-58) as it was a duplicate. The second instance with id: setup-mise is kept since it's needed for the Verify Mise Setup step that references it. Co-authored-by: bashandbone <[email protected]>
Remove duplicate Setup Mise Environment step in copilot-setup-steps workflow
…This was preventing package resolution
Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]>
… for better integration and testing
Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]>
…es and adjusting environment variables
Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]>
…s accessed in multiple places across the codebase as a method, when its 'computed_field' decorator made it a property. Fixed the incorrect calls and added property decorator for clarity. Also standardized optional dependency naming and switched to pydantic-ai-slim to correct a CI/CD problem for an unused dependency.
Short version: avoid debugging GH actions at all costs. * initial test * fix: enhance CI configuration with Python version support and Mise environment setup * fix: enhance CI configuration with Python version support and Mise environment setup * fix: prevent CI skip by disabling env export in mise-action The mise-action was hanging during the "Exporting mise environment variables" step, causing the runner to kill the job with exit code 137 (SIGKILL). This was happening because: 1. The mise.toml has complex template expressions with command executions 2. The hooks.enter configuration may execute during env export 3. The env export was timing out after ~1 minute Solution: Disable the env parameter (set to false) in all mise-action configurations to prevent the hanging during environment variable export. Fixes #issue * fix: remove unnecessary echo statement for Mise Python version in CI setup * fix: add mise bin directory to PATH for proper command execution The previous fix disabled env export to prevent hanging, but this caused MISE_PATH to be empty because mise wasn't in the PATH. Changes: 1. Add mise bin directory to PATH in "Set outputs" step 2. Add mise bin directory to PATH in workflow steps that execute mise commands This ensures: - The "Set outputs" step can find mise using which - Workflow steps can execute mise commands via $MISE_PATH - No hanging during env export (env: false is still set) Fixes the "command not found" error when running mise commands. * fix: add outputs section to composite action for proper output exposure The composite action was setting outputs in the mise-outputs step, but those outputs were not being exposed by the action itself because the action.yml was missing the outputs section. Changes: - Add outputs section to action.yml defining all output variables - Map outputs from mise-outputs step to action outputs This ensures that steps.setup-mise.outputs.MISE_PATH and other outputs are properly available to the calling workflow. Related to the CI skip issue fix. * fix: missing tools on path * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * Initial plan * fix: remove duplicate Setup Mise Environment step Remove the first instance of "Setup Mise Environment" step (lines 52-58) as it was a duplicate. The second instance with id: setup-mise is kept since it's needed for the Verify Mise Setup step that references it. Co-authored-by: bashandbone <[email protected]> * fix: update PATH in workflows and improve MCP Registry submission process * fix: removed 'frozen' flag from uv for experimental python installs. This was preventing package resolution * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: remove mcp-publisher CLI installation step and streamline submission process * fix: enhance CI workflows by adding outputs and environment variables for better integration and testing * fix: update Python setup action to include outputs for PYTHON_PATH and UV_PATH * fix: update CI workflows to ensure uv is available when called * fix: add tools to mise check task for CI availability * fix: add setup step for mise dev installs in CI * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: improve CI workflows by setting PATH for mise and cleaning up temporary files * Update .github/workflows/mcp-registry-submit.yml Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: enhance CI workflows by adding NEWPATH output and updating PATH setup * fix: update cloud setup task environment variables for consistency * fix: enhance setup and testing workflows by adding missing dependencies and adjusting environment variables * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: improve cleanup process in MCP Registry submission workflow * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: add missing outputs to actions and simplify CI workflows Root cause: Custom actions were missing `outputs:` sections in action.yml. Steps wrote to $GITHUB_OUTPUT but outputs weren't exposed to callers. Changes: - Add outputs section to setup-mise-env action (MISE_PATH, PYTHON_PATH, etc.) - Add outputs section to setup-python-env action (PYTHON_PATH, UV_PATH) - Add outputs section to setup-uv-env action (uv-path, uvx-path) - Simplify reusable workflows to use `mise run task` directly - Remove complex $MISE_PATH variable usage (mise is already in PATH) - Fix experimental Python tests: remove --frozen flag that fails with prereleases - Clean up redundant env vars (move to job level) - Remove duplicate disk space cleanup in copilot-setup-steps * Fix: Add missing 'runs-on' specification for lint job Signed-off-by: Adam Poulemanos <[email protected]> * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: update CI workflows and environment setup for consistency * fix: fixed a malformed block in setup-mise-env, consolidated freethreaded handling into normal workflows using environment variables * fix: update hk installation in CI setup and add HK_MISE environment variable * fix: update CI workflows and environment setup, remove unused variables * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: update setup-mise-env and reusable-lint workflows for consistency and improved output handling * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * fix: streamline mise configuration by consolidating tool definitions and removing unused venv creation arguments * fix: incorrect-tool references in mise.toml * fix: add mise-debug input for enhanced output in setup-mise-env action and update reusable test workflow * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * Change condition for inserting Free-Threaded Python correct use of nonexistent 'matches' Signed-off-by: Adam Poulemanos <[email protected]> * Add MISE_PYTHON_COMPILE variable to action.yml Signed-off-by: Adam Poulemanos <[email protected]> * fix: remove minor version check for uv * fix: update dependency references for type checking to use latest version * fix: streamline system dependency installation and update free-threaded Python setup * fix: add command to list available Python versions in mise setup * fix: correct command for creating virtual environment in mise.toml * fix: remove compile settings to experiment if cause of a CI failure. * fix: refine free-threaded Python installation process in CI setup * fix: enhance cloud setup script with informative messages and streamline mise commands * fix: add message to ensure Python version is installed during cloud setup * fix: improve cloud setup script to ensure mise activation and Python version installation * fix: enhance cloud setup with mise doctor output and ensure latest uv installation * fix: enhance cloud setup with improved mise activation and debugging output * fix: remove auto-venv to debug CI issue * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> --------- Signed-off-by: Adam Poulemanos <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]>
* docs: add connection pooling implementation plan Create detailed implementation plan for HTTP client connection pooling based on the client-pooling-analysis document. The plan outlines: - Phase 1: Infrastructure (HttpClientPool class, config settings, state integration) - Phase 2: Provider integration (Voyage AI, Cohere, Qdrant considerations) - Testing strategy and rollback plan - Provider-specific connection pool settings This addresses connection overhead, potential exhaustion during high-load indexing, and Qdrant timeout issues (httpcore.ReadError). * feat: implement HTTP client connection pooling for providers Add centralized HTTP client pooling infrastructure to improve connection reuse across Voyage AI and Cohere providers during high-load operations. Key changes: - Create HttpClientPool class with singleton pattern for application-wide connection reuse (src/codeweaver/common/http_pool.py) - Integrate http_pool into CodeWeaverState with proper cleanup on shutdown - Update ProviderRegistry to inject pooled httpx clients for Voyage/Cohere - Add provider-specific settings (50 max connections, 90s read timeout) - Add comprehensive unit tests for HttpClientPool This addresses: - Connection overhead during batch embedding operations - Potential connection exhaustion during large indexing jobs - Improved reliability with HTTP/2 support for better multiplexing The implementation includes graceful fallbacks - if pooling fails, providers will create their own clients as before. * refactor: address PR #194 review suggestions for HTTP connection pooling - Add thread safety via asyncio.Lock for coroutine-safe client creation - Make reset_http_pool() async to properly close clients before reset - Add reset_http_pool_sync() for testing fixtures without cleanup - Unify dual singleton pattern (remove redundant module-level instance) - Change INFO logging to DEBUG for client creation (reduce noise) - Narrow exception types (httpx.HTTPError, OSError) instead of Exception - Merge duplicate VOYAGE/COHERE conditionals using _POOLED_HTTP_PROVIDERS - Use get_client_sync() in provider registry for sync context - Add comprehensive tests: - Test override settings are actually applied to clients - Test aclose() error handling (graceful degradation) - Test PoolTimeouts immutability - Test concurrent get_client calls (thread safety) - Test _get_pooled_httpx_client for various providers - Update implementation plan status to Implemented - Export reset_http_pool_sync in common/__init__.py * feat: expand HTTP connection pooling to OpenAI-compatible and Mistral providers Extend connection pooling support to 11 providers: - OpenAI-compatible (8): OpenAI, Azure, Fireworks, Groq, Together, Ollama, Cerebras, Heroku - Voyage and Cohere (existing) - Mistral (new) Changes: - OpenAI factory: Accept http_client parameter for AsyncOpenAI - Mistral provider: Accept httpx_client mapped to async_client - Provider registry: Convert _POOLED_HTTP_PROVIDERS to dict mapping provider -> param name - Registry _instantiate_client: Use provider-specific parameter names - Tests: Add tests for OpenAI, Mistral pooling; update provider mapping tests - Docs: Update implementation plan with full provider table Provider parameter mapping: - httpx_client: Voyage, Cohere, Mistral - http_client: OpenAI, Azure, Fireworks, Groq, Together, Ollama, Cerebras, Heroku Not supported (no httpx injection): - Bedrock (uses boto3) - HuggingFace (global factory pattern) - Google GenAI (args only, not full client) * fix: add thread safety to singleton and sync client creation Address Copilot review suggestions from PR #194: 1. get_instance() race condition: Add double-checked locking with threading.Lock to prevent multiple singleton instances when called concurrently from different threads 2. get_client_sync() race condition: Add thread-safe double-checked locking pattern using _sync_lock to prevent duplicate client creation when called concurrently during provider initialization 3. Remove unused 'patch' import from test_http_pool.py Thread safety guarantees are now documented in the module docstring: - Singleton: threading.Lock with double-checked locking - Async clients: asyncio.Lock for coroutine safety - Sync clients: threading.Lock for thread safety * fix: resolve test failures for HTTP connection pooling - Mark _POOLED_HTTP_PROVIDERS as ClassVar to prevent Pydantic treating it as a private attribute - Fix import path: use codeweaver.providers.provider for ProviderKind instead of non-existent codeweaver.providers.kinds - Remove _limits assertions from tests since httpx.AsyncClient doesn't expose limits directly (timeout assertions are sufficient) * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]> * docs: document client caching behavior for HTTP pool Add Note section to get_client() and get_client_sync() docstrings clarifying that clients are cached by name only, not by override parameters. Subsequent calls with different overrides will return the originally cached client. This is intentional behavior for connection pooling - each provider should use consistent settings, which ProviderRegistry ensures. * fix: address Copilot review suggestions for HTTP pool 1. asyncio.Lock event loop binding issue: - Changed _async_lock to be lazily created on first use - asyncio.Lock is bound to the event loop it's created in, so lazy creation ensures it's bound to the correct loop - Renamed _lock to _async_lock for clarity 2. Race condition in fast-path checks: - Changed if-check to try-except pattern in get_client() and get_client_sync() to handle client removal between check and return - Prevents KeyError if close_client/close_all removes client mid-check 3. Sync/async mixing warning: - Added Warning section to get_client_sync() docstring explaining that sync and async methods should not be mixed 4. Walrus operator clarity: - Replaced walrus operator with explicit get() and None check in provider.py for better readability --------- Signed-off-by: Adam Poulemanos <[email protected]> Co-authored-by: Claude <[email protected]> Co-authored-by: Copilot <[email protected]>
The previous fix disabled env export to prevent hanging, but this caused MISE_PATH to be empty because mise wasn't in the PATH. Changes: 1. Add mise bin directory to PATH in "Set outputs" step 2. Add mise bin directory to PATH in workflow steps that execute mise commands This ensures: - The "Set outputs" step can find mise using which - Workflow steps can execute mise commands via $MISE_PATH - No hanging during env export (env: false is still set) Fixes the "command not found" error when running mise commands.
Co-authored-by: Copilot <[email protected]> Signed-off-by: Adam Poulemanos <[email protected]>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @bashandbone, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
👋 Hey @bashandbone, Thanks for your contribution to codeweaver! 🧵You need to agree to the CLA first... 🖊️Before we can accept your contribution, you need to agree to our Contributor License Agreement (CLA). To agree to the CLA, please comment:
Those exact words are important1, so please don't change them. 😉 You can read the full CLA here: Contributor License Agreement ✅ @bashandbone has signed the CLA. 0 out of 3 committers have signed the CLA. Footnotes
|
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request implements HTTP connection pooling for provider connections and includes extensive GitHub Actions workflow improvements. The main fix addresses incorrect property access causing indexing failures by converting file_hash() method calls to file_hash property access throughout the codebase.
Key Changes:
- Implemented HTTP client connection pooling via
HttpClientPoolsingleton to reduce connection overhead during high-load operations - Fixed critical bug where
file_hash()was called as a method instead of accessed as a property (@computed_fielddecorator) - Renamed optional dependency groups from
provider-*to simpler names (e.g.,provider-anthropic→anthropic) - Updated multiple GitHub Actions workflows with better environment handling, path management, and mise integration
Reviewed changes
Copilot reviewed 58 out of 67 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/codeweaver/common/http_pool.py |
New HTTP client pool manager with singleton pattern for connection reuse |
src/codeweaver/core/discovery.py |
Fixed file_hash property access (was incorrectly being called as method) |
src/codeweaver/engine/indexer/indexer.py |
Updated to use file_hash property instead of method call |
src/codeweaver/common/registry/provider.py |
Added pooled HTTP client injection for 11 providers |
src/codeweaver/server/server.py |
Integrated HTTP pool into server state with proper cleanup |
tests/unit/common/test_http_pool.py |
Comprehensive test coverage for new HTTP pooling functionality |
pyproject.toml |
Renamed optional dependencies, updated versions (pydantic, cyclopts, fastembed) |
uv.lock |
Removed unused dependencies, updated package versions |
| Multiple provider files | Fixed error messages to escape brackets in pip install commands |
.github/workflows/* |
Improved workflow configurations with better environment handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Adam Poulemanos <[email protected]>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This pull request updates the GitHub Actions workflows and reusable action definitions to improve consistency, environment setup, and output handling for Python development and CI. The changes standardize input formatting, add useful outputs, enhance environment configuration, and streamline job steps for better reliability and clarity.
Action and workflow configuration improvements:
.github/actions/setup-mise-env/action.yml,.github/actions/setup-python-env/action.yml, and.github/actions/setup-uv-env/action.ymlby using double quotes for defaults and adding explicit outputs for paths and environment details. [1] [2] [3]devandreviewerprofiles in.github/actions/setup-mise-env/action.yml, including additional environment variables and explicit activation commands. [1] [2] [3]Reusable workflow improvements:
.github/workflows/_reusable-build.yml,.github/workflows/_reusable-lint.yml, and.github/workflows/_reusable-test.yml, removing unnecessary quotes and ensuring consistent formatting. [1] [2] [3]uvwithmise, for more reliable and reproducible builds and tests. [1] [2] [3]Minor fixes and cleanups:
.github/actionlint.ymland other files for improved linting and readability.These changes collectively make the CI/CD workflows more robust, easier to maintain, and better suited for multi-profile Python development and testing.