Implement New Features and add E2E Tests - #3
Conversation
Enhanced PersistentDict to save mutation hashes before yielding tracked objects, ensuring nested changes are properly detected and saved in both sync and async contexts. Updated ProxyObject type hints for better IDE inference. Added tests for persistence saving and proxy usage patterns. Updated .gitignore to exclude test persistence artifacts.
Introduces dynamic provider and protocol registration for file IO, allowing custom cloud storage endpoints (e.g., Minio) to be registered at runtime. Implements server-side optimized copy for files within the same provider, both sync and async, improving performance for intra-provider transfers. Adds tests and docker-compose setup for multi-Minio environments, updates dependencies for fileio/file, and refactors provider/accessor management to support dynamic providers.
Bump required Python version to >=3.10 and add support for 3.10, 3.11, and 3.12. Update pydantic and pydantic-settings to >=2.0.0, adjust dependency markers, and add dev dependencies for testing. Lock file updated to reflect these changes and new package versions.
Introduces a GitHub Actions workflow for E2E tests, adds e2e test suite for file, persistence, API client, pool, and settings modules. Updates async method imports for aiopath compatibility, improves KeyDB session error logging, and enhances Redis backend key handling. Updates .gitignore and pyproject.toml to support new test and dependency requirements.
The release workflow now triggers on successful completion of the E2E Tests workflow on the main branch, and only runs if the workflow dispatch or E2E Tests succeed. The tests workflow now only triggers on changes to version files or on pull requests to main, improving workflow efficiency.
Introduces a Docker Compose file for local e2e testing with MinIO and Redis, and adds a Makefile target to automate setup and teardown. Refactors e2e MinIO tests and fixtures to use boto3 for bucket management, ensures proper event loop and session cleanup, and updates test code to use async/await directly. Updates .gitignore to exclude .log files, and makes minor fixes in logging, config, and async method creation for improved compatibility and reliability.
Introduces logic to automatically create S3/Minio buckets if they do not exist when mkdir or amkdir is called, handling both sync and async cases. The patch is applied to AWS, Minio, and S3C accessors. Adds an end-to-end test to verify that amkdir creates buckets as needed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| import datetime | ||
| from lzl.types import eproperty | ||
| from pydantic.types import ByteSize | ||
| from ast import Import |
There was a problem hiding this comment.
Bug: Unused and incorrect import from ast module
The import from ast import Import appears to be an accidental auto-import that was committed. The Import class from the ast module is not used anywhere in this file. This looks like an IDE auto-import artifact that was mistakenly included.
|
|
||
| import anyio | ||
| anyio.run(_test_thread_pool) | ||
| anyio.run(_test) |
There was a problem hiding this comment.
Bug: Test function never executes async code, passes vacuously
The test_thread_pool function defines inner async functions _test and _test_thread_pool but never actually executes them. The anyio.run calls on lines 36-37 are placed inside _test, which itself is never called. This means the test will pass without testing anything. The anyio.run(_test) call needs to be at the end of test_thread_pool, outside of _test.
| --health-retries 5 | ||
| # Start command is essential for MinIO | ||
| volumes: | ||
| - /tmp/minio_data:/data |
There was a problem hiding this comment.
Bug: MinIO services missing required server command
Both minio and minio2 service containers have a comment stating "Start command is essential for MinIO" but no command is actually specified. The MinIO Docker image requires a command like server /data to start the server. Without this, the containers will not serve S3 requests. The docker-compose.e2e.yml file correctly includes command: server /data --console-address ":9090" but the GitHub Actions workflow is missing this configuration.
Additional Locations (1)
There was a problem hiding this comment.
Pull request overview
This pull request implements comprehensive E2E testing infrastructure and migrates the codebase to support Pydantic v2 and Python 3.10+. The PR includes new E2E tests for various components (file I/O, persistence, pooling, API clients, etc.), updates dependencies to newer versions, adds dynamic protocol registration for file systems, improves mutation tracking in persistence, and sets up CI/CD workflows with Docker services.
Key Changes
- Added comprehensive E2E test suite with Docker Compose environment for MinIO and Redis services
- Migrated from Pydantic v1 to v2 with updated configuration patterns using
SettingsConfigDict - Implemented dynamic file system protocol registration with environment-based configuration
- Enhanced file operations with server-side copy optimization and bucket auto-creation for S3-compatible storage
Reviewed changes
Copilot reviewed 43 out of 78 changed files in this pull request and generated 55 comments.
Show a summary per file
| File | Description |
|---|---|
tests/e2e/*.py |
New E2E test suite covering file I/O, persistence, pooling, API clients, and settings |
tests/e2e/conftest.py |
Test fixtures and service health checks for E2E environment |
tests/docker-compose.e2e.yml |
Docker services (MinIO, Redis) for E2E testing |
src/lzl/types/settings.py |
Pydantic v2 migration with SettingsConfigDict |
src/lzl/proxied/base.py |
Enhanced type hints for ProxyObject |
src/lzl/io/persistence/main.py |
Fixed mutation tracking to save hash before yielding |
src/lzl/io/persistence/backends/redis.py |
Fixed kwargs iteration bug |
src/lzl/io/file/spec/providers/s3.py |
Added bucket auto-creation and server-side copy optimization |
src/lzl/io/file/configs/providers.py |
Added from_env_prefix method for dynamic configuration |
src/lzl/io/file/configs/main.py |
Support for custom provider registration |
src/lzl/io/file/main.py |
Added register_protocol method for dynamic protocols |
pyproject.toml |
Updated Python requirement to 3.10+, Pydantic to v2, updated AWS/S3 dependencies |
.github/workflows/tests.yml |
New CI workflow for E2E tests with service containers |
Makefile |
Added run-e2e-local target for local E2E testing |
Comments suppressed due to low confidence (1)
src/lzl/io/file/types/base.py:12
- Import of 'Import' is not used.
from ast import Import
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| num_workers: Optional[int] = 12 | ||
| checksum_cache_ttl: Optional[int] = 60 * 60 * 24 * 1 # 1 days | ||
| enable_progress_bar: Optional[bool] = False | ||
| enable_progress_bar: Optional[bool] = False |
There was a problem hiding this comment.
Duplicate field definition. The field enable_progress_bar is defined twice on consecutive lines (lines 50 and 51), which will cause the first definition to be overridden.
| enable_progress_bar: Optional[bool] = False |
| import boto3 | ||
| import boto3.session | ||
| from ..compat.r2 import R2FileSystem | ||
| from ..compat.r2.filesys import R2FileSystem |
There was a problem hiding this comment.
Incorrect import path. The import from ..compat.r2 import R2FileSystem has been changed to from ..compat.r2.filesys import R2FileSystem, but this should be verified as working. The original import suggests R2FileSystem was directly in r2/__init__.py, but now it's being imported from a filesys submodule.
| async def _test(): | ||
| # Initialize pool | ||
| pool = ThreadPool | ||
| result = await pool.create_background_task(io_bound_task, 1) | ||
| assert result == 2 | ||
|
|
||
| # Map | ||
| async def _test_thread_pool(): | ||
| pool = ThreadPool(max_workers=2) | ||
|
|
||
| results = [] | ||
| async for res in pool.aiterate(io_bound_task, [1, 2, 3]): | ||
| results.append(res) | ||
|
|
||
| assert sorted(results) == [2, 3, 4] | ||
|
|
||
| import anyio | ||
| anyio.run(_test_thread_pool) |
There was a problem hiding this comment.
Test function never awaited. The async function _test_thread_pool is defined inside _test() but then awaited with anyio.run(_test_thread_pool) at line 36. However, _test() itself is already being awaited with anyio.run(_test) at line 37. This creates confusion about the execution order and may not work as intended since both are called with anyio.run() which starts new event loops.
| volumes: | ||
| - /tmp/minio_data:/data | ||
|
|
||
| minio2: | ||
| # image: minio/minio:RELEASE.2024-12-18T13-15-44Z | ||
| image: minio/minio:edge-cicd | ||
| ports: | ||
| - "9001:9000" | ||
| # - "9091:9090" | ||
| env: | ||
| MINIO_ROOT_USER: minioadmin | ||
| MINIO_ROOT_PASSWORD: minioadmin | ||
| options: >- | ||
| --health-cmd "curl --silent --fail http://localhost:9000/minio/health/live" | ||
| --health-interval 30s | ||
| --health-timeout 10s |
There was a problem hiding this comment.
Incorrect workflow configuration. The GitHub Actions workflow has minio2 service defined with port mapping "9001:9000" (line 52), but this conflicts with the minio service which already maps 9001:9001 (line 36). This will cause a port conflict. Additionally, both services use the same volume mount /tmp/minio_data:/data which could cause data corruption.
| --health-timeout 5s | ||
| --health-retries 5 | ||
|
|
||
| minio: | ||
| # image: minio/minio:RELEASE.2024-12-18T13-15-44Z | ||
| image: minio/minio:edge-cicd | ||
| ports: | ||
| - 9000:9000 | ||
| # - 9001:9001 | ||
| env: | ||
| MINIO_ROOT_USER: minioadmin | ||
| MINIO_ROOT_PASSWORD: minioadmin | ||
| options: >- | ||
| --health-cmd "curl --silent --fail http://localhost:9000/minio/health/live" | ||
| --health-interval 30s | ||
| --health-timeout 10s |
There was a problem hiding this comment.
Missing command for MinIO services. The GitHub Actions MinIO services don't specify the server command in the options field. MinIO requires an explicit command like server /data to start properly. While this is specified in the docker-compose files, it's missing from the GitHub Actions service definition.
| # Cleanup | ||
| try: | ||
| path.delete() | ||
| except: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| afilesys = pd.backend.root.afilesys | ||
| if afilesys and hasattr(afilesys, 'session'): | ||
| await afilesys.session.close() | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| # So if fs_name is "mc2", provider becomes "mc2". | ||
| # build_s3c(provider="mc2") uses get_provider_config("mc2"). | ||
| # This should work if FileIOConfig.get_provider_config handles "mc2". | ||
| pass |
There was a problem hiding this comment.
Unnecessary 'pass' statement.
| pass |
| # But for now, let's assume if env var is set, it's there. | ||
| # Ideally we'd ping it. | ||
| pass | ||
| except Exception as e: |
There was a problem hiding this comment.
This statement is unreachable.
| # The loop closure error happens when something TRIES to use it. | ||
| # If we clear it, nothing should use it. | ||
| pass | ||
| except Exception: |
There was a problem hiding this comment.
This statement is unreachable.
| export MINIO2_ACCESS_KEY=minioadmin; \ | ||
| export MINIO2_SECRET_KEY=minioadmin; \ | ||
| export MINIO2_REGION=us-east-1; \ | ||
| export REDIS_URL=redis://localhost:6389/0; \ |
There was a problem hiding this comment.
Bug: Incorrect Redis port in local e2e Makefile
The Makefile run-e2e-local target sets REDIS_URL=redis://localhost:6389/0, but the GitHub Actions workflow in .github/workflows/tests.yml uses REDIS_URL: redis://localhost:6379/0. The docker-compose.e2e.yml maps port 6389:6379, so the Makefile is correct for local runs but this inconsistency between CI and local environments may cause confusion or test failures if someone copies the wrong URL.
| class Config: | ||
| env_prefix: str = "" | ||
| case_sensitive: bool = False | ||
| arbitrary_types_allowed: bool = True |
There was a problem hiding this comment.
Bug: Moving Config class breaks set_envvars for Pydantic v2
The Config class was moved inside the else block (for PYDANTIC_VERSION < 2), but the set_envvars method at line 169 still references self.Config.env_prefix. When running with Pydantic v2+, the Config class doesn't exist, causing an AttributeError when set_envvars is called. The code needs to access model_config['env_prefix'] for Pydantic v2 or handle both cases.
Note
Introduces cloud file I/O (S3/R2), Redis-backed persistence, and comprehensive E2E tests with CI workflows for tests and releases.
src/lzl/io/file/**(providers:s3,compat/r2).configs/*) and types/specs (spec/*,types/base.py).src/lzl/io/persistence/backends/redis.pyand wiring insrc/lzl/io/persistence/main.py.src/lzl/types/settings.py,src/lzl/proxied/base.py).src/lazyops/libs/logging/main.py,src/lazyops/utils/lazy.py).tests/_basic/**,tests/e2e/**).tests/_basic/docker-compose.yml,tests/docker-compose.e2e.yml)..github/workflows/tests.yml,release.yml).pyproject.toml,pytest.ini,.gitignore,Makefile_setup.py,GEMINI.md).Written by Cursor Bugbot for commit df39ea0. This will update automatically on new commits. Configure here.