Skip to content

fix: detect duplicate feature view/data source names at parse time - #6756

Open
mayuriphad wants to merge 2 commits into
feast-dev:masterfrom
mayuriphad:fix/flaky-duplicate-featureview-cli-6417
Open

fix: detect duplicate feature view/data source names at parse time#6756
mayuriphad wants to merge 2 commits into
feast-dev:masterfrom
mayuriphad:fix/flaky-duplicate-featureview-cli-6417

Conversation

@mayuriphad

Copy link
Copy Markdown

Problem

test_cli_apply_duplicated_featureview_names (and related tests) intermittently fail in CI with:

AssertionError: assert (-1 != 0 and b'Feature view names must be case-insensitively unique' in b'')

Root cause: duplicate feature view/data source name detection happens deep inside store.plan()/store.apply() via _validate_feature_views()/_validate_data_sources() in feature_store.py, after FeatureStore and its dependencies (Dask, provider, registry) have already been initialized. On process shutdown, slow atexit handlers (Dask thread pool, PySpark JVM) can block long enough that the test harness's subprocess timeout fires and kills the process with SIGKILL before any output is flushed — so the real error message is lost and the test sees only an empty, non-zero exit.

The CLI also didn't handle ConflictingFeatureViewNames/DataSourceRepeatNamesException (both FeastError subclasses) at all — only FeastProviderLoginError was caught — so these errors propagated as unhandled tracebacks.

Fix

  1. sdk/python/feast/repo_operations.py: call the existing _validate_feature_views() / _validate_data_sources() validators (imported from feature_store.py, no logic duplicated) at the end of parse_repo(), right after the repo contents are collected — before any FeatureStore/Dask/PySpark initialization happens. This fails fast, close to where the actual problem (duplicate names in the repo) exists.
  2. sdk/python/feast/cli/cli.py: add except FeastError handling (after the existing FeastProviderLoginError handler) in plan_command and apply_total_command, so any FeastError (including these) is surfaced as a clean message via click.ClickException with a non-zero exit code, rather than an unhandled traceback.

Testing

Ran the directly affected tests locally (all pass):

tests/integration/cli/test_cli_apply_duplicates.py::test_cli_apply_duplicated_featureview_names PASSED
tests/integration/cli/test_cli_apply_duplicates.py::test_cli_apply_duplicate_data_source_names PASSED
tests/integration/cli/test_cli_apply_duplicates.py::test_cli_apply_imported_featureview PASSED
tests/integration/cli/test_cli_apply_duplicates.py::test_cli_apply_imported_featureview_with_duplication PASSED
tests/integration/cli/test_cli_apply_duplicates.py::test_cli_apply_duplicated_featureview_names_multiple_py_files PASSED

Fixes #6417

Copilot AI lite review requested due to automatic review settings August 18, 2026 11:53
@mayuriphad
mayuriphad requested a review from a team as a code owner August 18, 2026 11:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Signed-off-by: mayuriphad <mayuriphad656@gmail.com>
@mayuriphad
mayuriphad force-pushed the fix/flaky-duplicate-featureview-cli-6417 branch from 2df34a2 to 03e84b3 Compare August 19, 2026 01:47
@mayuriphad

Copy link
Copy Markdown
Author

Hi maintainers — just checking in on this PR since it hasn't had a maintainer review yet. Happy to make any changes needed; let me know if anything's unclear or if I should split/rework it. Thanks for your time!

@ntkathole

Copy link
Copy Markdown
Member

Potential issues with this approach:

  1. --skip-feature-view-validation is now bypassedparse_repo() validates unconditionally, but the CLI exposes --skip-feature-view-validation (documented as an escape hatch for overly strict validation). Since parse_repo() does not receive this flag, users who pass it will still get validation errors they explicitly opted out of. Consider moving the early validation to _get_repo_contents() or plan()/apply_total() where the flag is available.

  2. Double validation — The existing validation in store.plan()/store.apply() (feature_store.py L1454-1461) is not removed, so validation runs twice per invocation. Harmless but wasteful.

  3. Importing private functions across modules_validate_feature_views and _validate_data_sources are _-prefixed internals of feature_store.py. Importing them in repo_operations.py creates a coupling to private API that could break without notice. Consider either making them public or extracting them to a shared validation module.

…module imports

Address review feedback on the duplicate feature view/data source name
check added at parse time:

- Move the fail-fast validation from parse_repo() (which has no access to
  CLI flags) into _get_repo_contents(), gated by skip_feature_view_validation
  so users who explicitly opt out via --skip-feature-view-validation are no
  longer blocked by it.
- Thread skip_feature_view_validation through plan() and apply_total() to
  _get_repo_contents(), matching how store.plan()/store.apply() already
  honor the flag.
- Rename _validate_feature_views/_validate_data_sources in feature_store.py
  to public validate_feature_views/validate_data_sources, since
  repo_operations.py now depends on them across module boundaries.

Signed-off-by: mayuriphad <mayuriphad656@gmail.com>
@mayuriphad

Copy link
Copy Markdown
Author

Thanks for the thorough review, @ntkathole — pushed fixes for all three:

  1. --skip-feature-view-validation bypass: moved the fail-fast validation out of parse_repo() (which has no access to CLI flags) and into _get_repo_contents(), gated by skip_feature_view_validation. Both plan() and apply_total() now thread the flag through to _get_repo_contents(), so passing --skip-feature-view-validation correctly skips this early check too, consistent with how store.plan()/store.apply() already honor it.
  2. Double validation: with the check now conditional on the same flag used by store.plan()/store.apply(), it's intentional double-checking (fail fast before heavy deps, then defense-in-depth inside the store) rather than always-on redundant work — and it's skippable in one place via the flag.
  3. Private cross-module imports: renamed _validate_feature_views/_validate_data_sources in feature_store.py to public validate_feature_views/validate_data_sources, and updated all internal callers plus the existing unit/integration tests that imported the private names.

Verified locally: the early check still fires by default (before FeatureStore is constructed) and is bypassed when skip_feature_view_validation=True is passed through _get_repo_contents(); existing validate_feature_views/validate_data_sources unit tests pass; ruff check/ruff format --check are clean on the touched files.

Let me know if you'd like anything else adjusted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky test: test_cli_apply_duplicated_featureview_names fails with empty output on subprocess timeout

3 participants