Sorry, something went wrong.
There was a problem hiding this comment.
This PR updates Feast’s developer workflow by adjusting pre-commit stages, refining Python lint/type-check configuration, and introducing new Makefile targets and helper scripts aimed at speeding up local development and test runs.
Changes:
Copilot reviewed 12 out of 17 changed files in this pull request and generated 13 comments.
Show a summary per file| sdk/python/tests/integration/feature_repos/universal/online_store/milvus.py | Modernize type hints for online store config return type. |
| sdk/python/tests/integration/feature_repos/repo_configuration.py | Widen typing for local online-store replacement configs. |
| sdk/python/pytest.ini | Add markers, reduce default timeout, and adjust warning filtering/addopts. |
| sdk/python/pyproject.toml | Enable MyPy incremental/sqlite cache and improve diagnostics. |
| sdk/python/feast/templates/snowflake/feature_repo/__init__.py | Add package marker file for template repo. |
| sdk/python/feast/templates/snowflake/bootstrap.py | Import formatting change in Snowflake template bootstrap. |
| sdk/python/feast/templates/snowflake/__init__.py | Add package marker file for Snowflake templates. |
| sdk/python/feast/infra/online_stores/singlestore_online_store/__init__.py | Add package marker file for infra module. |
| sdk/python/feast/infra/compute_engines/dag/__init__.py | Add package marker file for compute engine module. |
| sdk/python/feast/infra/compute_engines/aws_lambda/__init__.py | Add package marker file for compute engine module. |
| scripts/uv-run.sh | Add helper wrapper to run uv from the SDK directory. |
| scripts/perf-monitor.py | Add benchmarking script for formatting/lint/tests/precommit checks. |
| scripts/mypy-daemon.sh | Add helper to manage dmypy for faster type checks. |
| scripts/check-init-py.sh | Add script to detect missing __init__.py files under feast/. |
| Makefile | Update lint/format commands, add new targets, and modify test invocation paths. |
| .pre-commit-config.yaml | Move local hooks from push stage to commit stage. |
| .github/workflows/linter.yml | Add caching for .mypy_cache in the Python linter workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| MYPY_CACHE_DIR="${ROOT_DIR}/sdk/python/.mypy_cache" | ||
| PID_FILE="$MYPY_CACHE_DIR/dmypy.pid" | ||
|
|
||
| case "$1" in |
There was a problem hiding this comment.
With set -u, running this script without arguments will error on case "$1" in before showing the usage message. Use case "${1:-}" in (or an explicit arg-count check) so mypy-daemon.sh fails gracefully and prints usage.
| case "$1" in | |
| case "${1:-}" in |
Sorry, something went wrong.
| cd ${ROOT_DIR}/sdk/python; python -m ruff check feast/ tests/ | ||
| cd ${ROOT_DIR}/sdk/python; python -m ruff format --check feast/ tests | ||
|
|
||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/ruff ] && .venv/bin/ruff check feast/ tests/ || ruff check feast/ tests/) |
There was a problem hiding this comment.
lint-python no longer checks formatting (ruff format --check ...). Since the CI linter workflow runs make lint-python, this change would stop CI from enforcing formatting and could allow unformatted code to merge. Consider adding a ruff format --check step back into lint-python (or switching CI to run make precommit-check).
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/ruff ] && .venv/bin/ruff check feast/ tests/ || ruff check feast/ tests/) | |
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/ruff ] && .venv/bin/ruff check feast/ tests/ || ruff check feast/ tests/) | |
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/ruff ] && .venv/bin/ruff format --check feast/ tests/ || ruff format --check feast/ tests/) |
Sorry, something went wrong.
| test-python-integration-parallel: ## Run integration tests with enhanced parallelization | ||
| cd $(ROOT_DIR)/sdk/python && .venv/bin/python -m pytest tests/integration \ | ||
| -n auto --dist loadscope \ | ||
| --timeout=300 --tb=short -v \ | ||
| --integration --color=yes --durations=20 |
There was a problem hiding this comment.
These targets hardcode .venv/bin/python without the fallback to python used by other test targets. That makes make test-python-integration-parallel fail for developers/CI environments that install deps into a different venv or use --system installs. Consider using the same conditional fallback pattern as test-python-unit/test-python-integration.
Sorry, something went wrong.
| cd $(ROOT_DIR)/sdk/python && .venv/bin/python -m pytest --tb=short -v -n 8 --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ | ||
| -k "not test_lambda_materialization and not test_snowflake_materialization" \ | ||
| -m "not rbac_remote_integration_test" \ | ||
| --log-cli-level=INFO -s \ | ||
| sdk/python/tests | ||
| tests |
There was a problem hiding this comment.
This target now hardcodes .venv/bin/python (no fallback), but the docs commonly assume running tests from whatever Python environment is active (e.g. after pip install -e). Consider restoring the prior python -m pytest ... behavior or adding the same .venv/bin/python-or-python fallback used by test-python-unit.
Sorry, something went wrong.
| cd $(ROOT_DIR)/sdk/python && .venv/bin/python -m pytest --tb=short -v -n 8 --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ | ||
| -k "not test_lambda_materialization and not test_snowflake_materialization" \ | ||
| -m "rbac_remote_integration_test" \ | ||
| --log-cli-level=INFO -s \ | ||
| sdk/python/tests | ||
| tests |
There was a problem hiding this comment.
Same issue as test-python-integration-local: this target hardcodes .venv/bin/python with no fallback, which will break when deps are installed into a different environment (or when using uv --system). Please add the same conditional fallback pattern used elsewhere.
Sorry, something went wrong.
| @(docker info > /dev/null 2>&1 && \ | ||
| FEAST_LOCAL_ONLINE_CONTAINER=True \ | ||
| python -m pytest -n 8 --integration sdk/python/tests \ | ||
| cd $(ROOT_DIR)/sdk/python && .venv/bin/python -m pytest -n 8 --integration tests \ |
There was a problem hiding this comment.
This docker-based test target now assumes .venv/bin/python exists. If a developer runs this after installing dependencies into an active venv or system env (no .venv), it will fail. Consider adding the .venv/bin/python-or-python fallback used by other pytest targets.
| cd $(ROOT_DIR)/sdk/python && .venv/bin/python -m pytest -n 8 --integration tests \ | |
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/python ] && .venv/bin/python -m pytest -n 8 --integration tests || python -m pytest -n 8 --integration tests) \ |
Sorry, something went wrong.
| ignore::DeprecationWarning | ||
| ignore::PendingDeprecationWarning |
There was a problem hiding this comment.
filterwarnings now ignores all DeprecationWarning and PendingDeprecationWarning. This broadly suppresses deprecations from Feast code/tests too (previously these were scoped to specific noisy dependencies), which can hide real upgrade issues. Consider restoring scoped ignores for known noisy modules instead of disabling these warnings globally.
| ignore::DeprecationWarning | |
| ignore::PendingDeprecationWarning |
Sorry, something went wrong.
| install-python-dependencies-ci: ## Install Python CI dependencies using uv sync | ||
| uv sync --extra ci |
There was a problem hiding this comment.
🔴 CI dependency installation loses PyTorch CPU-only handling for Linux
The install-python-dependencies-ci Makefile target was changed from using explicit CPU-only PyTorch handling to a simple uv sync --extra ci.
Click to expandThe old implementation had explicit logic to:
Old code (Makefile:80-93 before change):
New code (Makefile:102-103):
Without this special handling on Linux CI, the build may:
The pyproject.toml includes pytorch dependency via the ci extra which pulls torch>=2.7.0, but doesn't specify the CPU-only index URL.
Was this helpful? React with 👍 or 👎 to provide feedback.
Sorry, something went wrong.
| if ( | ||
| "ikv" not in full_name | ||
| and "milvus" not in full_name | ||
| and "openlineage" not in full_name | ||
| and "ray_transformation" not in full_name | ||
| ): |
There was a problem hiding this comment.
🔴 Undefined temp_module variable used when module is skipped in doctest
When a module name contains ray_transformation (or other skip terms like ikv, milvus, openlineage), the temp_module variable is never assigned in that iteration, but the code unconditionally tries to use it later to create a DocTestSuite.
Root CauseThe temp_module assignment is inside a conditional block:
But later, temp_module is used unconditionally at line 109-111:
This causes two possible issues:
Impact: Tests may crash with NameError: name 'temp_module' is not defined, or worse, doctests may silently run against the wrong module, leading to misleading test results.
Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Sorry, something went wrong.
| lint-python: ## Lint Python code | ||
| cd ${ROOT_DIR}/sdk/python; python -m mypy feast | ||
| cd ${ROOT_DIR}/sdk/python; python -m ruff check feast/ tests/ | ||
| cd ${ROOT_DIR}/sdk/python; python -m ruff format --check feast/ tests |
There was a problem hiding this comment.
removing format check from CI?
Sorry, something went wrong.
There was a problem hiding this comment.
nah i'll add that back.
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good with nitpicks
Sorry, something went wrong.
|
https://github.com/feast-dev/feast/blob/master/.github/workflows/registry-rest-api-tests.yml#L147 to fix rest-api tests uv run python -m pytest sdk/python/tests/integration/registration/rest_api/test_registry_rest_api.py --integration -s |
Sorry, something went wrong.
| # Integration tests with better parallelization | ||
| test-python-integration-parallel: ## Run integration tests with enhanced parallelization | ||
| uv run python -m pytest sdk/python/tests/integration \ | ||
| -n auto --dist loadscope \ |
There was a problem hiding this comment.
🔴 test-python-integration-parallel target uses 300s timeout instead of 1200s like all other integration targets
The new test-python-integration-parallel Makefile target specifies --timeout=300 (5 minutes), while every other integration test target in the Makefile uses --timeout=1200 (20 minutes). Integration tests are inherently slower and frequently need more than 5 minutes per test. This will cause legitimate integration tests to be killed with timeout errors when run via this target.
Root Cause and ImpactAll existing integration test targets consistently use --timeout=1200:
But the new target at Makefile:199-203 uses --timeout=300:
The 300s value appears to have been copied from the new timeout = 300 setting in sdk/python/pytest.ini:21, which is intended as a default for unit tests. Integration tests need the longer 1200s timeout.
Impact: Integration tests running via this target will fail with timeout errors for any test taking over 5 minutes, which is common for integration tests that interact with external services.
| -n auto --dist loadscope \ | |
| --timeout=1200 --tb=short -v \ | |
Was this helpful? React with 👍 or 👎 to provide feedback.
Sorry, something went wrong.
| timeout = 300 | ||
| timeout_method = thread |
There was a problem hiding this comment.
🔴 Global timeout = 300 in pytest.ini silently breaks long-running integration test targets
The new timeout = 300 setting in pytest.ini applies a 5-minute per-test timeout to all test runs that discover this config file, not just unit tests. While some integration targets explicitly pass --timeout=1200 (which overrides the config), many do not and will now time out.
Affected Makefile targets and root causeThe pytest.ini at sdk/python/pytest.ini is discovered by pytest for any test run targeting sdk/python/tests. The following Makefile targets run integration tests without an explicit --timeout flag and will now inherit the 300s limit:
Previously, the old pytest.ini had no timeout setting, so these targets ran with no per-test timeout. Integration tests routinely exceed 5 minutes (the existing explicit timeouts use --timeout=1200, i.e., 20 minutes). With this change, individual integration tests that take longer than 300 seconds will be killed with a timeout error.
Impact: Long-running integration tests will fail with TimeoutError in CI and local development, even though the test logic is correct.
Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback.
Sorry, something went wrong.
This comprehensive update modernizes Feast's development workflow with significant performance improvements inspired by llama-stack patterns:
Precommit Hook Improvements:
Test Performance Optimizations:
New Developer Tools:
Expected Performance Gains:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Misc