-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: Modernize precommit hooks and optimize test performance #5929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
e7c9113
031a978
a23152e
ae52fcc
88cefb3
759dd9e
ff4548e
de69e92
c8bbf87
46ed9d9
df45285
6bc12c2
eb6b346
5417ab2
6f6c736
9dae77f
63c8f3b
2068303
0da7c1d
8848a45
4904104
60466b2
97cd848
386c7cf
d8b156c
0b6d274
c530cf6
dec75eb
f50366b
282558a
f8051e1
0e111fc
12b4a72
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -239,3 +239,5 @@ infra/website/dist/ | |
|
|
||
| # offline builds | ||
| offline_build/ | ||
| feast_profile_demo/ | ||
| scripts/perf-monitor.py | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,23 @@ | ||
| default_stages: | ||
| - push | ||
| default_stages: [commit] | ||
|
|
||
| repos: | ||
| - repo: local | ||
| hooks: | ||
| - id: format | ||
| name: Format | ||
| stages: [ push ] | ||
| stages: [commit] | ||
| language: system | ||
| entry: make format-python | ||
| pass_filenames: false | ||
| - id: lint | ||
| name: Lint | ||
| stages: [ push ] | ||
| stages: [commit] | ||
| language: system | ||
| entry: make lint-python | ||
| pass_filenames: false | ||
| - id: template | ||
| name: Build Templates | ||
| stages: [ commit ] | ||
| stages: [commit] | ||
| language: system | ||
| entry: make build-templates | ||
| pass_filenames: false | ||
| pass_filenames: false | ||
|
Comment on lines
1
to
23
|
||
| 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; python -m ruff check --fix feast/ tests/ | ||
| cd ${ROOT_DIR}/sdk/python; python -m ruff format feast/ tests/ | ||
| uv run ruff check --fix sdk/python/feast/ sdk/python/tests/ | ||
| uv run ruff format sdk/python/feast/ sdk/python/tests/ | ||
|
|
||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removing format check from CI?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nah i'll add that back. |
||
|
|
||
| uv run ruff check sdk/python/feast/ sdk/python/tests/ | ||
| uv run bash -c "cd sdk/python && 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 | ||
| uv run bash -c "cd sdk/python && 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 scripts/mypy-daemon.sh | ||
|
|
||
| ##@ Python SDK - local | ||
| # formerly install-python-ci-dependencies-uv-venv | ||
| # editable install | ||
|
|
@@ -74,23 +95,22 @@ install-python-dependencies-minimal: ## Install minimal Python dependencies usin | |
| uv pip sync --require-hashes sdk/python/requirements/py$(PYTHON_VERSION)-minimal-requirements.txt | ||
| uv pip install --no-deps -e .[minimal] | ||
|
|
||
| ##@ Python SDK - system | ||
| # the --system flag installs dependencies in the global python context | ||
| # instead of a venv which is useful when working in a docker container or ci. | ||
| ##@ Python SDK - CI (uses uv with virtualenv) | ||
| # Uses uv pip sync with virtualenv for CI environments | ||
|
|
||
| # Used in github actions/ci | ||
| # formerly install-python-ci-dependencies-uv | ||
| install-python-dependencies-ci: ## Install Python CI dependencies in system environment using uv | ||
| # Install CPU-only torch first to prevent CUDA dependency issues | ||
| pip uninstall torch torchvision -y || true | ||
| install-python-dependencies-ci: ## Install Python CI dependencies using uv pip sync | ||
| # Create virtualenv if it doesn't exist | ||
| uv venv .venv | ||
| # Install CPU-only torch first to prevent CUDA dependency issues (Linux only) | ||
| @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; \ | ||
| uv pip sync --extra-index-url https://download.pytorch.org/whl/cpu --index-strategy unsafe-best-match sdk/python/requirements/py$(PYTHON_VERSION)-ci-requirements.txt; \ | ||
| else \ | ||
| echo "Installing dependencies from PyPI for macOS..."; \ | ||
| uv pip sync --system sdk/python/requirements/py$(PYTHON_VERSION)-ci-requirements.txt; \ | ||
| uv pip sync sdk/python/requirements/py$(PYTHON_VERSION)-ci-requirements.txt; \ | ||
| fi | ||
| uv pip install --system --no-deps -e . | ||
| uv pip install --no-deps -e . | ||
|
|
||
| # Used in github actions/ci | ||
| install-hadoop-dependencies-ci: ## Install Hadoop dependencies | ||
|
|
@@ -151,22 +171,44 @@ 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) | ||
| python -m pytest -n 8 --color=yes $(if $(pattern),-k "$(pattern)") sdk/python/tests | ||
| uv run python -m pytest -n 8 --color=yes $(if $(pattern),-k "$(pattern)") sdk/python/tests | ||
|
|
||
| # Fast unit tests only | ||
| test-python-unit-fast: ## Run fast unit tests only (no external dependencies) | ||
| uv run python -m pytest sdk/python/tests/unit -n auto -x --tb=short | ||
|
|
||
| # Changed files only (requires pytest-testmon) | ||
| test-python-changed: ## Run tests for changed files only | ||
| uv run python -m pytest --testmon -n 8 --tb=short sdk/python/tests | ||
|
|
||
| # Quick smoke test for PRs | ||
| test-python-smoke: ## Quick smoke test for development | ||
| uv run python -m pytest \ | ||
| sdk/python/tests/unit/test_unit_feature_store.py \ | ||
| sdk/python/tests/unit/test_repo_operations_validate_feast_project_name.py \ | ||
| -n 4 --tb=short | ||
|
|
||
| test-python-integration: ## Run Python integration tests (CI) | ||
| python -m pytest --tb=short -v -n 8 --integration --color=yes --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ | ||
| uv run 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 \ | ||
| sdk/python/tests | ||
|
|
||
| # 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 \ | ||
| --timeout=300 --tb=short -v \ | ||
| --integration --color=yes --durations=20 | ||
devin-ai-integration[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
199
to
203
|
||
|
|
||
| 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 \ | ||
| python -m pytest --tb=short -v -n 8 --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ | ||
| uv run 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 \ | ||
|
|
@@ -175,7 +217,7 @@ test-python-integration-local: ## Run Python integration tests (local dev mode) | |
| test-python-integration-rbac-remote: ## Run Python remote RBAC integration tests | ||
| FEAST_IS_LOCAL_TEST=True \ | ||
| FEAST_LOCAL_ONLINE_CONTAINER=True \ | ||
| python -m pytest --tb=short -v -n 8 --color=yes --integration --durations=10 --timeout=1200 --timeout_method=thread --dist loadgroup \ | ||
| uv run 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 \ | ||
|
|
@@ -184,7 +226,7 @@ test-python-integration-rbac-remote: ## Run Python remote RBAC integration tests | |
| test-python-integration-container: ## Run Python integration tests using Docker | ||
| @(docker info > /dev/null 2>&1 && \ | ||
| FEAST_LOCAL_ONLINE_CONTAINER=True \ | ||
| python -m pytest -n 8 --integration sdk/python/tests \ | ||
| uv run python -m pytest -n 8 --integration sdk/python/tests \ | ||
| ) || 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 +262,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 +598,7 @@ test-python-universal-couchbase-online: ## Run Python Couchbase online store int | |
| sdk/python/tests | ||
|
|
||
| test-python-universal: ## Run all Python integration tests | ||
| python -m pytest -n 8 --integration sdk/python/tests | ||
| uv run python -m pytest -n 8 --integration sdk/python/tests | ||
|
|
||
| ##@ Java | ||
|
|
||
|
|
@@ -622,7 +664,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 | ||
| 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 . | ||
|
|
@@ -727,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 | ||
| ##@ Go SDK | ||
| PB_REL = https://github.com/protocolbuffers/protobuf/releases | ||
| PB_VERSION = 30.2 | ||
| PB_ARCH := $(shell uname -m) | ||
|
|
@@ -798,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 . | ||
|
|
||
| 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" |
| 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.
Show resolved
Hide resolved
|
||||||
| case "$1" in | |
| case "${1:-}" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.