GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix: Run store.plan() only when need it. by shuchu · Pull Request #3708 · feast-dev/feast

Conversation

shuchu commented Aug 3, 2023

Copy link

Copy Markdown

Collaborator

What this PR does / why we need it:

This line of code:
registry_diff, infra_diff, new_infra = store.plan(repo)

is always executed while we run "feast apply". It will call the validate() method by default which makes the option "-skip-source-validation" invalid.

Based on the original code logic, I change it to run it only when it is required. The requirement comes from:
if store._should_use_plan():

In the implementation of _should_use_plan():

def _should_use_plan(self):   
"""Returns True if plan and _apply_diffs should be used, False otherwise."""   
"""# Currently only the local provider with sqlite online store supports plan and _apply_diffs. """  
return self.config.provider == "local" and ( self.config.online_store and self.config.online_store.type == "sqlite" )`

the "store._should_use_plan()" return True while the provider is "local" and the online store is "sqlite".

With this PR, we can alleviate the issue 3682. If a user wants to do local development, he/she may want to change the online store to a different one other than "SQLite", the action of double call "store.validate()" can be avoided.

Which issue(s) this PR fixes:
Fixes # 3682

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix: issue-3682, run store.plan() only when need it.

c3801c3

shuchu changed the title ~~fix: issue-3682, run store.plan() only when need it.~~ fix: run store.plan() only when need it.

Aug 3, 2023

shuchu changed the title ~~fix: run store.plan() only when need it.~~ fix: Run store.plan() only when need it.

Aug 3, 2023

felixwang9817 approved these changes

Aug 14, 2023

View reviewed changes

felixwang9817 left a comment

Copy link

Copy Markdown

Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

/lgtm

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

felixwang9817 merged commit 7bc7c47 into feast-dev:master

Aug 14, 2023

felixwang9817 pushed a commit that referenced this pull request

Aug 14, 2023


          chore(release): release 0.33.1

1ff9aee

## [0.33.1](v0.33.0...v0.33.1) (2023-08-14)

### Bug Fixes

* Run store.plan() only when need it. ([#3708](#3708)) ([7bc7c47](7bc7c47))

adchia pushed a commit that referenced this pull request

Sep 7, 2023


          chore(release): release 0.34.0

a8aeee9

# [0.34.0](v0.33.0...v0.34.0) (2023-09-07)

### Bug Fixes

* Add NUMERIC to bq_to_feast type map ([#3719](#3719)) ([6474b4b](6474b4b))
* Fix python unit tests ([#3734](#3734)) ([e81684d](e81684d))
* Handle unknown postgres source types gracefully ([#3634](#3634)) ([d7041f4](d7041f4))
* Pin protobuf version to avoid seg fault on some machines ([028cc20](028cc20))
* Remove unwanted excessive splitting of gcs path, so expected gcs parquet paths are returned from BigQueryRetrievalJob.to_remote_storage() ([#3730](#3730)) ([f2c5988](f2c5988))
* Run store.plan() only when need it. ([#3708](#3708)) ([7bc7c47](7bc7c47))
* Saved datasets no longer break CLI registry-dump command ([#3717](#3717)) ([f28ccc2](f28ccc2))
* Update py3.8 ci requirements for cython 3.0 release ([#3735](#3735)) ([1695c13](1695c13))

### Features

* Enhance customization of Trino connections when using Trino-based Offline Stores ([#3699](#3699)) ([ed7535e](ed7535e))
* Implement gRPC server to ingest streaming features ([#3687](#3687)) ([a3fcd1f](a3fcd1f))

zseta pushed a commit to zseta/feast that referenced this pull request

Feb 7, 2024


          fix: Run store.plan() only when need it. (feast-dev#3708)

db80a2b

fix: issue-3682, run store.plan() only when need it.
Signed-off-by: Attila Toth <[email protected]>

zseta pushed a commit to zseta/feast that referenced this pull request

Feb 7, 2024


          chore(release): release 0.34.0

08e11b9

# [0.34.0](feast-dev/feast@v0.33.0...v0.34.0) (2023-09-07)

### Bug Fixes

* Add NUMERIC to bq_to_feast type map ([feast-dev#3719](feast-dev#3719)) ([6474b4b](feast-dev@6474b4b))
* Fix python unit tests ([feast-dev#3734](feast-dev#3734)) ([e81684d](feast-dev@e81684d))
* Handle unknown postgres source types gracefully ([feast-dev#3634](feast-dev#3634)) ([d7041f4](feast-dev@d7041f4))
* Pin protobuf version to avoid seg fault on some machines ([028cc20](feast-dev@028cc20))
* Remove unwanted excessive splitting of gcs path, so expected gcs parquet paths are returned from BigQueryRetrievalJob.to_remote_storage() ([feast-dev#3730](feast-dev#3730)) ([f2c5988](feast-dev@f2c5988))
* Run store.plan() only when need it. ([feast-dev#3708](feast-dev#3708)) ([7bc7c47](feast-dev@7bc7c47))
* Saved datasets no longer break CLI registry-dump command ([feast-dev#3717](feast-dev#3717)) ([f28ccc2](feast-dev@f28ccc2))
* Update py3.8 ci requirements for cython 3.0 release ([feast-dev#3735](feast-dev#3735)) ([1695c13](feast-dev@1695c13))

### Features

* Enhance customization of Trino connections when using Trino-based Offline Stores ([feast-dev#3699](feast-dev#3699)) ([ed7535e](feast-dev@ed7535e))
* Implement gRPC server to ingest streaming features ([feast-dev#3687](feast-dev#3687)) ([a3fcd1f](feast-dev@a3fcd1f))

Signed-off-by: Attila Toth <[email protected]>

shuchu mentioned this pull request

Feb 26, 2024

The --skip-source-validation option not working #3682

Closed

EXPEbdodla pushed a commit to ExpediaGroup/feast that referenced this pull request

Jun 10, 2024


          fix: Run store.plan() only when need it. (feast-dev#3708)

5a882ee

fix: issue-3682, run store.plan() only when need it.

EXPEbdodla pushed a commit to ExpediaGroup/feast that referenced this pull request

Jun 11, 2024


          fix: Run store.plan() only when need it. (feast-dev#3708)

0ec6755

fix: issue-3682, run store.plan() only when need it.

EXPEbdodla added a commit to ExpediaGroup/feast that referenced this pull request

Jun 11, 2024


          Revert "fix: Run store.plan() only when need it. (feast-dev#3708)"

27617b8

This reverts commit 0ec6755.

Reviewers

felixwang9817

felixwang9817 approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.