fix: Surface FeatureViewPinConflict and version changes during feast plan#6391
Open
Abhishek8108 wants to merge 1 commit intofeast-dev:masterfrom
Open
Conversation
…plan Resolves feast-dev#6104 Three gaps existed in the plan-mode versioning flow: 1. FeatureViewPinConflict was never raised during `feast plan` — the check only ran inside apply_feature_view. Users could not discover pin conflicts until after running `feast apply`. 2. The `version` spec field was compared as a raw proto string in diff_registry_objects, producing unhelpful output like "latest -> v2". The resolved numeric pin target (from meta.current_version_number) was never shown. 3. No tests covered plan-mode behaviour for versioned feature views. Changes: - BaseRegistry.check_version_pin_conflict(): new read-only method that mirrors the conflict detection in apply_feature_view. Uses get_feature_view_by_version and get_any_feature_view (both available on BaseRegistry) so it works for both file and SQL registries without duplicating per-registry logic. - store.plan(): calls check_version_pin_conflict for every feature view (FeatureView, StreamFeatureView, OnDemandFeatureView) in the desired repo contents, surfacing conflicts before apply runs. - diff_registry_objects(): excludes `version` from the generic spec field loop (added to FIELDS_TO_IGNORE) and adds a dedicated version display block that reads meta.current_version_number for the current state, producing output like "v2 (pin) -> v1 (pin)" or "v1 (pin) -> latest". - 8 new unit tests across TestCheckVersionPinConflict and TestVersionDiffDisplay covering: latest-version no-op, forward declarations, schema-only changes, pin+schema conflicts, pin display, unpin display, and plan-loop wiring. Signed-off-by: Abhishek8108 <87538407+Abhishek8108@users.noreply.github.com>
Contributor
Author
|
The Failing test:
This is the same pattern as the |
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
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.
Summary
Closes #6104
Three gaps existed in the plan-mode versioning flow introduced by #6101:
1.
FeatureViewPinConflictnever raised duringfeast planThe conflict check (simultaneous version pin + schema change) only ran inside
apply_feature_view(). Users had no way to discover pin conflicts without runningfeast apply— defeating the purpose of plan mode.2. Version changes not visible in plan output
diff_registry_objectsiterated overspecfields and comparedversionas a raw proto string ("latest" -> "v2"). The resolved numeric pin target frommeta.current_version_numberwas never surfaced, so users could not see which numeric version they were pinning to or from.3. No tests for plan-mode versioning
No unit tests covered either of the above.
Changes
BaseRegistry.check_version_pin_conflict()(new method)Read-only plan-time guard that mirrors the conflict detection inside
apply_feature_view. Implemented onBaseRegistryusingget_feature_view_by_versionandget_any_feature_view— both already declared onBaseRegistry— so it works for both file and SQL registries without duplicating per-registry logic.Conflict condition: user is simultaneously pinning to an existing version AND the feature view definition differs from the active version (schema or UDF change).
store.plan()— calls the conflict checkIterates all feature views (
FeatureView,StreamFeatureView,OnDemandFeatureView) indesired_repo_contentsand callsregistry.check_version_pin_conflict()for each, surfacingFeatureViewPinConflictbeforefeast applyruns.diff_registry_objects()— human-readable version display"version"toFIELDS_TO_IGNORE(excluded from generic spec field loop)meta.current_version_numberfor the current state andspec.versionfor the desired state, producing output like:Test plan
TestCheckVersionPinConflict::test_latest_version_no_conflict— no-op forlatestTestCheckVersionPinConflict::test_forward_declaration_no_conflict— pinning to a non-existent version (forward declaration) does not raiseTestCheckVersionPinConflict::test_pin_no_schema_change_no_conflict— pinning to an existing version without schema change does not raiseTestCheckVersionPinConflict::test_pin_with_schema_change_raises— pinning to an existing version while also changing schema raisesFeatureViewPinConflictTestVersionDiffDisplay::test_no_version_change_no_version_diff— identical FVs produce no version property diffTestVersionDiffDisplay::test_pin_shows_in_diff— pinning shows"v1 (pin)"in declared valueTestVersionDiffDisplay::test_unpin_shows_in_diff— unpinning shows"v1 (pin)"in existing value and"latest"in declaredTestVersionDiffDisplay::test_plan_calls_pin_conflict_check— plan loop callscheck_version_pin_conflictfor each FVruff format --checkandruff check— clean on all 4 modified files