feat: Modernize precommit hooks and optimize test performance #5929
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,23 @@ | ||
| default_stages: [commit] | ||
|
|
||
| repos: | ||
| - repo: local | ||
| hooks: | ||
| - id: format | ||
| name: Format | ||
| stages: [commit] | ||
| language: system | ||
| entry: make format-python | ||
| pass_filenames: false | ||
| - id: lint | ||
| name: Lint | ||
| stages: [commit] | ||
| language: system | ||
| entry: make lint-python | ||
| pass_filenames: false | ||
| - id: template | ||
| name: Build Templates | ||
| stages: [commit] | ||
| language: system | ||
| entry: make build-templates | ||
| pass_filenames: false | ||
|
franciscojavierarceo marked this conversation as resolved.
|
||
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
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -55,14 +55,35 @@ protos: compile-protos-python compile-protos-docs ## Compile protobufs for Pytho | |||||||
| build: protos build-docker ## Build protobufs and Docker images | ||||||||
|
|
||||||||
| format-python: ## Format Python code | ||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/ruff ] && .venv/bin/ruff check --fix feast/ tests/ || ruff check --fix feast/ tests/) | ||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/ruff ] && .venv/bin/ruff format feast/ tests/ || ruff format feast/ tests/) | ||||||||
|
|
||||||||
| lint-python: ## Lint Python code | ||||||||
|
Member
There was a problem hiding this comment. removing format check from CI?
Member
Author
There was a problem hiding this comment. nah i'll add that back. |
||||||||
| 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.
Suggested change
|
||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/mypy ] && .venv/bin/mypy feast || mypy feast) | ||||||||
|
|
||||||||
| # New combined target | ||||||||
| precommit-check: format-python lint-python ## Run all precommit checks | ||||||||
| @echo "✅ All precommit checks passed" | ||||||||
|
|
||||||||
| # Install precommit hooks with correct stages | ||||||||
| install-precommit: ## Install precommit hooks (runs on commit, not push) | ||||||||
| pip install pre-commit | ||||||||
| pre-commit install --hook-type pre-commit | ||||||||
| @echo "✅ Precommit hooks installed (will run on commit, not push)" | ||||||||
|
|
||||||||
| # Manual full type check | ||||||||
| mypy-full: ## Full MyPy type checking with all files | ||||||||
| cd ${ROOT_DIR}/sdk/python && uv run --no-project mypy feast tests | ||||||||
|
|
||||||||
| # Run precommit on all files | ||||||||
| precommit-all: ## Run all precommit hooks on all files | ||||||||
| pre-commit run --all-files | ||||||||
|
|
||||||||
| # Make scripts executable | ||||||||
| setup-scripts: ## Make helper scripts executable | ||||||||
| chmod +x scripts/uv-run.sh scripts/check-init-py.sh | ||||||||
|
devin-ai-integration[bot] marked this conversation as resolved.
Outdated
There was a problem hiding this comment.
Suggested change
|
||||||||
|
|
||||||||
| ##@ Python SDK - local | ||||||||
| # formerly install-python-ci-dependencies-uv-venv | ||||||||
| # editable install | ||||||||
|
|
@@ -80,9 +101,7 @@ install-python-dependencies-minimal: ## Install minimal Python dependencies usin | |||||||
|
|
||||||||
| # Used in github actions/ci | ||||||||
| # formerly install-python-ci-dependencies-uv | ||||||||
| install-python-dependencies-ci: ## Install Python CI dependencies using uv (system) | ||||||||
| @if [ "$$(uname -s)" = "Linux" ]; then \ | ||||||||
| echo "Installing dependencies with torch CPU index for Linux..."; \ | ||||||||
| uv pip sync --system --extra-index-url https://download.pytorch.org/whl/cpu --index-strategy unsafe-best-match sdk/python/requirements/py$(PYTHON_VERSION)-ci-requirements.txt; \ | ||||||||
|
|
@@ -151,40 +170,69 @@ benchmark-python-local: ## Run integration + benchmark tests for Python (local d | |||||||
| ##@ Tests | ||||||||
|
|
||||||||
| test-python-unit: ## Run Python unit tests (use pattern=<pattern> to filter tests, e.g., pattern=milvus, pattern=test_online_retrieval.py, pattern=test_online_retrieval.py::test_get_online_features_milvus) | ||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/python ] && .venv/bin/python -m pytest -n 8 --color=yes $(if $(pattern),-k "$(pattern)") tests || python -m pytest -n 8 --color=yes $(if $(pattern),-k "$(pattern)") tests) | ||||||||
|
|
||||||||
| # Fast unit tests only | ||||||||
| test-python-unit-fast: ## Run fast unit tests only (no external dependencies) | ||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/python ] && .venv/bin/python -m pytest tests/unit -n auto -x --tb=short || python -m pytest tests/unit -n auto -x --tb=short) | ||||||||
|
|
||||||||
| # Changed files only (requires pytest-testmon) | ||||||||
| test-python-changed: ## Run tests for changed files only | ||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/python ] && .venv/bin/python -m pytest --testmon -n 8 --tb=short tests || python -m pytest --testmon -n 8 --tb=short tests) | ||||||||
|
|
||||||||
| # Quick smoke test for PRs | ||||||||
| test-python-smoke: ## Quick smoke test for development | ||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/python ] && .venv/bin/python -m pytest \ | ||||||||
| tests/unit/test_unit_feature_store.py \ | ||||||||
| tests/unit/test_repo_operations_validate_feast_project_name.py \ | ||||||||
| -n 4 --tb=short || python -m pytest \ | ||||||||
| tests/unit/test_unit_feature_store.py \ | ||||||||
| tests/unit/test_repo_operations_validate_feast_project_name.py \ | ||||||||
| -n 4 --tb=short) | ||||||||
|
|
||||||||
| test-python-integration: ## Run Python integration tests (CI) | ||||||||
| cd $(ROOT_DIR)/sdk/python && ([ -f .venv/bin/python ] && .venv/bin/python -m pytest --tb=short -v -n 8 --integration --color=yes --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ | ||||||||
| -k "(not snowflake or not test_historical_features_main)" \ | ||||||||
| -m "not rbac_remote_integration_test" \ | ||||||||
| --log-cli-level=INFO -s \ | ||||||||
| tests || python -m pytest --tb=short -v -n 8 --integration --color=yes --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ | ||||||||
| -k "(not snowflake or not test_historical_features_main)" \ | ||||||||
| -m "not rbac_remote_integration_test" \ | ||||||||
| --log-cli-level=INFO -s \ | ||||||||
| tests) | ||||||||
|
|
||||||||
| # Integration tests with better parallelization | ||||||||
| 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 \ | ||||||||
|
Contributor
There was a problem hiding this comment. 🔴 The new Root Cause and ImpactAll existing integration test targets consistently use
But the new target at The 300s value appears to have been copied from the new 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.
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||
| --timeout=300 --tb=short -v \ | ||||||||
| --integration --color=yes --durations=20 | ||||||||
|
devin-ai-integration[bot] marked this conversation as resolved.
Comment on lines
+200
to
+204
There was a problem hiding this comment. These targets hardcode |
||||||||
|
|
||||||||
| test-python-integration-local: ## Run Python integration tests (local dev mode) | ||||||||
| FEAST_IS_LOCAL_TEST=True \ | ||||||||
| FEAST_LOCAL_ONLINE_CONTAINER=True \ | ||||||||
| HADOOP_HOME=$$HOME/hadoop \ | ||||||||
| CLASSPATH="$$( $$HADOOP_HOME/bin/hadoop classpath --glob ):$$CLASSPATH" \ | ||||||||
| HADOOP_USER_NAME=root \ | ||||||||
| 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 \ | ||||||||
| tests | ||||||||
|
There was a problem hiding this comment. This target now hardcodes |
||||||||
|
|
||||||||
| test-python-integration-rbac-remote: ## Run Python remote RBAC integration tests | ||||||||
| FEAST_IS_LOCAL_TEST=True \ | ||||||||
| FEAST_LOCAL_ONLINE_CONTAINER=True \ | ||||||||
| 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 \ | ||||||||
| tests | ||||||||
|
There was a problem hiding this comment. Same issue as |
||||||||
|
|
||||||||
| test-python-integration-container: ## Run Python integration tests using Docker | ||||||||
| @(docker info > /dev/null 2>&1 && \ | ||||||||
| FEAST_LOCAL_ONLINE_CONTAINER=True \ | ||||||||
| 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
Suggested change
|
||||||||
| ) || echo "This script uses Docker, and it isn't running - please start the Docker Daemon and try again!"; | ||||||||
|
|
||||||||
| test-python-universal-spark: ## Run Python Spark integration tests | ||||||||
|
|
@@ -220,7 +268,7 @@ test-python-historical-retrieval: | |||||||
| test_historical_features_persisting or \ | ||||||||
| test_historical_retrieval_fails_on_validation" \ | ||||||||
| sdk/python/tests | ||||||||
|
|
||||||||
| test-python-universal-trino: ## Run Python Trino integration tests | ||||||||
| PYTHONPATH='.' \ | ||||||||
| FULL_REPO_CONFIGS_MODULE=sdk.python.feast.infra.offline_stores.contrib.trino_repo_configuration \ | ||||||||
|
|
@@ -556,7 +604,7 @@ test-python-universal-couchbase-online: ## Run Python Couchbase online store int | |||||||
| sdk/python/tests | ||||||||
|
|
||||||||
| test-python-universal: ## Run all Python integration tests | ||||||||
| cd $(ROOT_DIR)/sdk/python && .venv/bin/python -m pytest -n 8 --integration tests | ||||||||
|
franciscojavierarceo marked this conversation as resolved.
Outdated
|
||||||||
|
|
||||||||
| ##@ Java | ||||||||
|
|
||||||||
|
|
@@ -622,7 +670,7 @@ build-feature-transformation-server-docker: ## Build Feature Transformation Serv | |||||||
| push-feature-server-java-docker: ## Push Feature Server Java Docker image | ||||||||
| docker push $(REGISTRY)/feature-server-java:$(VERSION) | ||||||||
|
|
||||||||
| build-feature-server-java-docker: ## Build Feature Server Java Docker image | ||||||||
| docker buildx build --build-arg VERSION=$(VERSION) \ | ||||||||
| -t $(REGISTRY)/feature-server-java:$(VERSION) \ | ||||||||
| -f java/infra/docker/feature-server/Dockerfile --load . | ||||||||
|
|
@@ -721,12 +769,12 @@ build-ui-local: ## Build Feast UI locally | |||||||
| cd $(ROOT_DIR)/ui && yarn install && npm run build --omit=dev | ||||||||
| rm -rf $(ROOT_DIR)/sdk/python/feast/ui/build | ||||||||
| cp -r $(ROOT_DIR)/ui/build $(ROOT_DIR)/sdk/python/feast/ui/ | ||||||||
|
|
||||||||
| format-ui: ## Format Feast UI | ||||||||
| cd $(ROOT_DIR)/ui && NPM_TOKEN= yarn install && NPM_TOKEN= yarn format | ||||||||
|
|
||||||||
|
|
||||||||
| ##@ Go SDK | ||||||||
| PB_REL = https://github.com/protocolbuffers/protobuf/releases | ||||||||
| PB_VERSION = 30.2 | ||||||||
| PB_ARCH := $(shell uname -m) | ||||||||
|
|
@@ -792,4 +840,3 @@ build-go-docker-dev: ## Build Go Docker image for development | |||||||
| docker buildx build --build-arg VERSION=dev \ | ||||||||
| -t feastdev/feature-server-go:dev \ | ||||||||
| -f go/infra/docker/feature-server/Dockerfile --load . | ||||||||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| #!/bin/bash | ||
| # Check for missing __init__.py files in Python packages | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| ROOT_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" | ||
|
|
||
| # Find Python package directories missing __init__.py | ||
| missing_init_files=() | ||
|
|
||
| while IFS= read -r -d '' dir; do | ||
| # Skip .ipynb_checkpoints directories and other unwanted directories | ||
| if [[ "${dir}" == *".ipynb_checkpoints"* ]] || [[ "${dir}" == *"__pycache__"* ]]; then | ||
| continue | ||
| fi | ||
|
|
||
| if [[ ! -f "${dir}/__init__.py" ]] && [[ -n "$(find "${dir}" -maxdepth 1 -name "*.py" -print -quit)" ]]; then | ||
| missing_init_files+=("${dir}") | ||
| fi | ||
| done < <(find "${ROOT_DIR}/sdk/python/feast" -type d -print0) | ||
|
|
||
| if [[ ${#missing_init_files[@]} -gt 0 ]]; then | ||
| echo "❌ Missing __init__.py files in:" | ||
| printf " %s\n" "${missing_init_files[@]}" | ||
| echo "" | ||
| echo "Run: touch ${missing_init_files[*]/%//__init__.py}" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "✅ All Python packages have __init__.py files" |
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||
| #!/bin/bash | ||||||
| # MyPy daemon for sub-second type checking | ||||||
|
|
||||||
| set -euo pipefail | ||||||
|
|
||||||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||||||
| ROOT_DIR="$(cd "${SCRIPT_DIR}/.." && pwd)" | ||||||
|
|
||||||
| MYPY_CACHE_DIR="${ROOT_DIR}/sdk/python/.mypy_cache" | ||||||
| PID_FILE="$MYPY_CACHE_DIR/dmypy.pid" | ||||||
|
|
||||||
| case "$1" in | ||||||
|
franciscojavierarceo marked this conversation as resolved.
Outdated
There was a problem hiding this comment. With
Suggested change
|
||||||
| start) | ||||||
| echo "🚀 Starting MyPy daemon..." | ||||||
| cd ${ROOT_DIR}/sdk/python | ||||||
| uv run dmypy start -- --config-file=pyproject.toml | ||||||
| echo "✅ MyPy daemon started" | ||||||
| ;; | ||||||
| check) | ||||||
| echo "🔍 Running MyPy daemon check..." | ||||||
| cd ${ROOT_DIR}/sdk/python | ||||||
| time uv run dmypy check feast tests | ||||||
| ;; | ||||||
| stop) | ||||||
| echo "🛑 Stopping MyPy daemon..." | ||||||
| cd ${ROOT_DIR}/sdk/python | ||||||
| uv run dmypy stop | ||||||
| echo "✅ MyPy daemon stopped" | ||||||
| ;; | ||||||
| restart) | ||||||
| echo "🔄 Restarting MyPy daemon..." | ||||||
| $0 stop | ||||||
| $0 start | ||||||
| ;; | ||||||
| status) | ||||||
| echo "📊 MyPy daemon status:" | ||||||
| cd ${ROOT_DIR}/sdk/python | ||||||
| if uv run dmypy status; then | ||||||
| echo "✅ MyPy daemon is running" | ||||||
| else | ||||||
| echo "❌ MyPy daemon is not running" | ||||||
| fi | ||||||
| ;; | ||||||
| *) | ||||||
| echo "Usage: $0 {start|check|stop|restart|status}" | ||||||
| echo "" | ||||||
| echo "Commands:" | ||||||
| echo " start - Start the MyPy daemon" | ||||||
| echo " check - Run type checking with the daemon" | ||||||
| echo " stop - Stop the MyPy daemon" | ||||||
| echo " restart - Restart the daemon" | ||||||
| echo " status - Check daemon status" | ||||||
| exit 1 | ||||||
| ;; | ||||||
| esac | ||||||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.