fix: detect duplicate feature view/data source names early in parse_repo to fix flaky test (#6417)#6454
Conversation
…epo (feast-dev#6417) Moves duplicate feature view name and data source name detection into parse_repo() before FeatureStore initialization. This prevents flaky test failures caused by slow atexit handlers (Dask, PySpark) blocking process exit when the error is raised late.
Add sys import and FeastError catch blocks to plan_command and apply_total_command in cli.py. This ensures clean error output and immediate exit (code 1) when validation errors like ConflictingFeatureViewNames are raised, preventing unhandled traceback and slow process exit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbcf25aaf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| lower_name = fv.name.lower() | ||
| if lower_name in fv_names_seen: | ||
| existing_fv = fv_names_seen[lower_name] | ||
| raise ConflictingFeatureViewNames( |
There was a problem hiding this comment.
Honor skip-feature-view-validation during early parsing
When feast plan/apply --skip-feature-view-validation is used, _get_repo_contents() still calls parse_repo() before the flag reaches FeatureStore.plan()/apply(), so this unconditional duplicate feature-view check now fails even though those methods explicitly skip _validate_all_feature_views under that option. Repos relying on the documented escape hatch for overly strict feature-view validation will no longer be able to run the command; pass the skip flag into parsing or keep this validation on the existing gated path.
Useful? React with 👍 / 👎.
|
@coded review |
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(), afterFeatureStoreand heavy dependencies (Dask, registry, provider) had already been initialized. When the validation error was raised, slowatexithandlers (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.pyConflictingFeatureViewNamesandDataSourceRepeatNamesExceptionimports fromfeast.errorsparse_repo()before anyFeatureStoreinitialization:FeatureView,StreamFeatureView,OnDemandFeatureView) are case-insensitively unique → raisesConflictingFeatureViewNamesimmediately if collision foundDataSourceRepeatNamesExceptionimmediately if collision foundsdk/python/feast/cli/cli.pyimport sysFeastErrorto the import fromfeast.errorsexcept FeastErrorhandler (after existingFeastProviderLoginError) in bothplan_commandandapply_total_command— prints a clean error message and exits with code 1 instead of letting validation errors propagate as unhandled tracebacksHow has this been tested
sdk/python/tests/integration/cli/test_cli_apply_duplicates.pycover the fix:test_cli_apply_duplicated_featureview_namestest_cli_apply_duplicate_data_source_namestest_cli_apply_duplicated_featureview_names_multiple_py_filesThese tests assert
rc != 0and the expected error message in the output, which is now guaranteed since the error is raised before any heavy initialization.Closes
Closes #6417