Skip to content

fix: Detect duplicate feature view/data source names early in parse_repo (#6417) - #6466

Open
obielin wants to merge 2 commits into
feast-dev:masterfrom
obielin:fix/early-duplicate-validation-6417-signed
Open

fix: Detect duplicate feature view/data source names early in parse_repo (#6417)#6466
obielin wants to merge 2 commits into
feast-dev:masterfrom
obielin:fix/early-duplicate-validation-6417-signed

Conversation

@obielin

@obielin obielin commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What this PR does / why we need it

Fixes the flaky test test_cli_apply_duplicated_featureview_names (and similar tests) that intermittently fails in CI with empty output due to subprocess timeout.

Root cause: Duplicate feature view name detection happened too late in the pipeline — inside store.plan() / store.apply() via _validate_feature_views(), after FeatureStore and heavy dependencies (Dask, registry, provider) had already been initialized. When the validation error was raised, slow atexit handlers (Dask thread pool) could block process exit indefinitely, causing the 60s timeout to trigger and the output to be lost.

Changes

sdk/python/feast/repo_operations.py

  • Added ConflictingFeatureViewNames and DataSourceRepeatNamesException imports from feast.errors
    • Added early duplicate detection in parse_repo() before any FeatureStore initialization:
    • Validates that all feature view names (across FeatureView, StreamFeatureView, OnDemandFeatureView) are case-insensitively unique → raises ConflictingFeatureViewNames immediately if collision found
    • Validates that all data source names are case-insensitively unique → raises DataSourceRepeatNamesException immediately if collision found

sdk/python/feast/cli/cli.py

  • Added import sys
    • Added FeastError to the import from feast.errors
    • Added except FeastError handler (after existing FeastProviderLoginError) in both plan_command and apply_total_command — prints a clean error message and exits with code 1 instead of letting validation errors propagate as unhandled tracebacks

How has this been tested

  • The existing integration tests in sdk/python/tests/integration/cli/test_cli_apply_duplicates.py cover the fix:
    • test_cli_apply_duplicated_featureview_names
    • test_cli_apply_duplicate_data_source_names
    • test_cli_apply_duplicated_featureview_names_multiple_py_files
      These tests assert rc != 0 and the expected error message in the output, which is now guaranteed since the error is raised before any heavy initialization.

Closes

Closes #6417

@obielin
obielin requested a review from a team as a code owner June 2, 2026 14:14
devin-ai-integration[bot]
devin-ai-integration Bot reviewed Jun 2, 2026
View reviewed changes

@devin-ai-integration devin-ai-integration Bot 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.

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment thread sdk/python/feast/cli/cli.py Outdated
@obielin
obielin force-pushed the fix/early-duplicate-validation-6417-signed branch 2 times, most recently from c58993b to 4b34498 Compare June 6, 2026 07:58
@obielin

obielin commented Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

Hi @ntkathole and @franciscojavierarceo — this is the updated, clean PR replacing #6454, which was closed due to DCO sign-off issues. All 5 commits are now properly signed off (DCO ✅), the import bug in cli.py has been fixed, and there are no merge conflicts. Would appreciate a review when you get a chance!

@obielin
obielin force-pushed the fix/early-duplicate-validation-6417-signed branch 2 times, most recently from 2023a56 to b874bba Compare June 13, 2026 13:25
obielin added 2 commits August 8, 2026 15:55
@obielin
fix: Detect duplicate FV/data source names early in parse_repo (feast…
531a50b 
…-dev#6417)

Move duplicate feature view name and data source name detection into parse_repo() before FeatureStore initialization. Also add FeastError handler in CLI plan/apply commands for clean error output.

Signed-off-by: Linda Oraegbunam <108290852+obielin@users.noreply.github.com>
Signed-off-by: Linda Oraegbunam <obielinda@gmail.com>
@obielin
fix: Add FeastError handler and sys import in cli.py (feast-dev#6417)
e0884bf 
Add import sys, FeastError import, and except FeastError handlers in plan_command and apply_total_command for clean error output and sys.exit(1).

Signed-off-by: Linda Oraegbunam <108290852+obielin@users.noreply.github.com>
Signed-off-by: Linda Oraegbunam <obielinda@gmail.com>
@obielin
obielin force-pushed the fix/early-duplicate-validation-6417-signed branch from b874bba to e0884bf Compare August 8, 2026 14:55
@obielin

obielin commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Hi @ntkathole friendly ping on this one. I've just rebased onto the latest master: no conflicts, DCO green.

On why there's no test signal here: this PR is from a fork, so the workflow runs are sitting in "awaiting approval", and pr-integration-tests / pr-registration-integration-tests are additionally gated on the ok-to-test / lgtm / approved label, so they've been skipping in ~1s since June. Would you mind approving the workflow runs and adding ok-to-test so the unit tests and integration suites can actually run against this?

A few points I'd like your steer on, since they're the parts most likely to need a maintainer decision. The except FeastError handler in plan_command / apply_total_command is deliberately broad — I can narrow it to ConflictingFeatureViewNames / DataSourceRepeatNamesException if you'd rather keep full tracebacks for unrelated Feast errors. I also added sys.exit(1) to the existing FeastProviderLoginError handler so login failures stop exiting 0, which is slightly beyond the issue scope and I'm happy to drop it into a separate PR. The new check in parse_repo() runs unconditionally, so it isn't affected by --skip-feature-view-validation let me know if it should respect that flag. And duplicate-name detection would now live in parse_repo, feature_store._validate_feature_views and the registry, so I'm glad to factor it into a shared helper either here or as a follow-up.

I can also add a unit test asserting that parse_repo() raises before any FeatureStore initialisation, rather than relying only on the existing CLI tests in sdk/python/tests/unit/cli/test_cli_apply_duplicates.py. Just say the word and I'll push it.

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

Reviewers

@devin-ai-integration devin-ai-integration[bot] devin-ai-integration[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

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

1 participant

@obielin