Enhance dependency management and testing framework #20
Conversation
- Updated Dockerfile to include `--extra=all` in the `uv sync` command for comprehensive dependency resolution. - Expanded Makefile to add a new target for running dependency tests. - Modified `pyproject.toml` to define optional dependencies and ensure the `all` extra includes all individual extras. - Improved README.md and added TESTING.md to document the new testing framework for dependency resolution consistency. - Introduced a new test script and Python test file to validate that all dependency combinations resolve to the same lock file. - Updated FastAPI endpoints to handle request headers more effectively.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @bwebs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the project's dependency management by introducing a robust optional dependency system and a new automated testing framework to ensure dependency resolution consistency. It also includes an important update to FastAPI endpoint header handling for improved reliability and comprehensive documentation for these changes.
Highlights
- Dependency Management Refinement: I've restructured
pyproject.tomlto define optional dependency groups (mcp,embed-observability,user-attribute-updater) and a comprehensiveallextra. This ensures thatuv sync --extra=allcorrectly installs all optional components, providing a clearer and more modular dependency structure. - Automated Dependency Consistency Testing: I've introduced a new, robust testing framework, including a Python test script (
tests/test_dependency_resolution.py), a shell script (tests/test_deps.sh), and aMakefiletarget (make test-deps). This framework automatically verifies that installing all individual optional dependencies yields the same lock file as installing theallextra, ensuring consistency and preventing unexpected dependency conflicts. - Dockerfile Update: The Dockerfile has been updated to utilize
uv sync --extra=allduring the image build process. This change ensures that all necessary optional dependencies are included in the Docker image, streamlining deployment and ensuring comprehensive functionality. - FastAPI Header Handling Improvement: I've refactored FastAPI endpoints and the
UserAttributeUpdaterclass to handle request headers more effectively. Instead of directrequest.headers.get(), the code now iterates overrequest.headers.items()for more robust and explicit header processing. - Enhanced Documentation: I've improved the
README.mdwith a new section detailing the optional dependencies and their usage. Additionally, newTESTING.mdfiles (in the root andtests/directory) have been added to comprehensively document the new dependency resolution testing framework, including how to run tests and troubleshoot issues.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the project's dependency management and testing framework. The introduction of optional dependencies in pyproject.toml and the uv sync --extra=all command in the Dockerfile are great improvements for flexibility and ease of use. The new testing framework, including the Python test script and shell script, along with the comprehensive TESTING.md documentation, greatly improves the maintainability and reliability of dependency resolution. The refactoring of get_request_authorization_for_value to decouple it from FastAPI's Request object is also a positive step towards better modularity. The review comments primarily focus on improving the robustness and maintainability of the newly introduced tests and documentation. This review adheres to commonly used Python style guides, such as PEP 8.
tests/test_dependency_resolution.py
Outdated
| assert '"mcp[cli]>=1.9.2"' in content, "mcp dependency not found in 'all' extra" | ||
| assert '"fastapi>=0.115.12"' in content, "fastapi dependency not found in 'all' extra" | ||
| assert '"selenium>=4.32.0"' in content, "selenium dependency not found in 'all' extra" | ||
| assert '"duckdb>=1.2.2"' in content, "duckdb dependency not found in 'all' extra" |
There was a problem hiding this comment.
The checks for specific dependencies within the all extra (e.g., "mcp[cli]>=1.9.2" in content) are hardcoded. While this ensures these specific dependencies are present, it doesn't dynamically verify that the all extra contains the union of all other individual extras. If a new dependency is added to an individual extra (like mcp or embed-observability), this test won't automatically fail unless these hardcoded checks are manually updated. Consider dynamically extracting dependencies from each individual extra and asserting that the all extra's dependencies are a superset of this combined list for better maintainability.
| # Testing Dependency Resolution | ||
|
|
||
| This project includes tests to ensure that all dependency combinations resolve to the same lock file, maintaining consistency in the dependency tree. | ||
|
|
||
| ## What the tests verify | ||
|
|
||
| The tests ensure that these two commands produce identical lock files: | ||
|
|
||
| 1. `uv sync --extra all` | ||
| 2. `uv sync --extra [all individual extras]` | ||
|
|
||
| Where "all individual extras" are dynamically discovered from `pyproject.toml` (excluding the `all` extra itself). | ||
|
|
||
| This is important because the `all` extra should be equivalent to installing all individual extras together. | ||
|
|
||
| ## Running the tests | ||
|
|
||
| ### Option 1: Shell script (recommended for quick testing) | ||
|
|
||
| ```bash | ||
| ./tests/test_deps.sh | ||
| ``` | ||
|
|
||
| This script will: | ||
| - Dynamically discover all individual extras from `pyproject.toml` | ||
| - Create temporary directories | ||
| - Run both dependency resolution commands | ||
| - Compare the resulting lock files | ||
| - Clean up automatically | ||
|
|
||
| ### Option 2: Python test (for CI/CD integration) | ||
|
|
||
| ```bash | ||
| # Run directly | ||
| python tests/test_dependency_resolution.py | ||
|
|
||
| # Run with pytest | ||
| uv run pytest tests/test_dependency_resolution.py -v | ||
| ``` | ||
|
|
||
| ### Option 3: Makefile target | ||
|
|
||
| ```bash | ||
| make test-deps | ||
| ``` | ||
|
|
||
| ## What the tests check | ||
|
|
||
| 1. **Dynamic Extra Discovery**: Automatically discovers all individual extras from `pyproject.toml` | ||
| 2. **Dependency Resolution Consistency**: Ensures that `--extra all` and `--extra [individual extras]` resolve to the same set of packages | ||
| 3. **Configuration Validation**: Verifies that all required extras are properly defined in `pyproject.toml` | ||
| 4. **Lock File Integrity**: Compares SHA256 hashes of generated lock files | ||
|
|
||
| ## Expected output | ||
|
|
||
| When the tests pass, you should see: | ||
|
|
||
| ``` | ||
| 🔍 Discovering individual extras... | ||
| 📦 Found individual extras: mcp embed-observability user-attribute-updater | ||
| 🎉 SUCCESS: All dependency combinations resolve to the same lock file! | ||
| ✅ Dependency resolution is consistent | ||
| ``` | ||
|
|
||
| ## Adding new extras | ||
|
|
||
| When you add a new extra to `pyproject.toml`, the tests will automatically: | ||
|
|
||
| 1. Discover the new extra | ||
| 2. Include it in the dependency resolution test | ||
| 3. Verify that `--extra all` still includes all dependencies from the new extra | ||
|
|
||
| No manual test updates are required! | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| If the tests fail, it usually means: | ||
|
|
||
| 1. **Missing dependencies**: The `all` extra doesn't include all the dependencies from individual extras | ||
| 2. **Version conflicts**: Different dependency combinations resolve to different versions |
There was a problem hiding this comment.
It appears that the content of this TESTING.md file is duplicated in the root TESTING.md. Having two identical documentation files can lead to confusion and make it difficult to keep them synchronized. It's best practice to have a single source of truth for documentation.
Given that other test-related files (test_dependency_resolution.py, test_deps.sh) are located in the tests/ directory, it makes sense for this documentation to reside here as well. Please consider removing the TESTING.md file from the project root.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Added development dependencies for `pytest` and `ruff` in `pyproject.toml`. - Removed outdated test files `test_dependency_resolution.py` and `TESTING.md`. - Refactored `test_dependency_resolution.py` to improve dependency resolution consistency checks and ensure the 'all' extra includes all non-dev dependencies from individual extras. - Updated `uv.lock` with new package versions and dependencies. This commit enhances the testing framework and ensures better management of optional dependencies.
- Deleted `pytest` and `ruff` from the `dev` section in `pyproject.toml` to streamline dependency management.
- Removed `dev` dependencies (`pytest` and `ruff`) from `uv.lock` and adjusted the `provides-extras` section accordingly. - Updated GitHub Actions workflow to use `--extra all` for dependency installation, ensuring all necessary dependencies are included for testing.
--extra=allin theuv synccommand for comprehensive dependency resolution.pyproject.tomlto define optional dependencies and ensure theallextra includes all individual extras.