fix: Detect duplicate feature view/data source names early in parse_repo (#6417)#6466
Open
obielin wants to merge 2 commits into
Open
fix: Detect duplicate feature view/data source names early in parse_repo (#6417)#6466obielin wants to merge 2 commits into
obielin wants to merge 2 commits into
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
obielin
force-pushed
the
fix/early-duplicate-validation-6417-signed
branch
2 times, most recently
from
c58993b to
4b34498
Compare
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 |
obielin
force-pushed
the
fix/early-duplicate-validation-6417-signed
branch
from
d753347 to
2023a56
Compare
…-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>
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
force-pushed
the
fix/early-duplicate-validation-6417-signed
branch
from
2023a56 to
b874bba
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
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