Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: issue-3682: run store.plan() only when we need it. #3691

Closed
wants to merge 1 commit into from

Conversation

shuchu
Copy link
Collaborator

@shuchu shuchu commented Jul 18, 2023

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 4682. 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

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: shuchu
To complete the pull request process, please assign niklasvm after the PR has been reviewed.
You can assign the PR to them by writing /assign @niklasvm in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@InstructorZhang
Copy link

Thanks for raising this PR. BTW, the issue number is 3682 rather than 4682.

@shuchu shuchu closed this Jul 19, 2023
@shuchu shuchu deleted the issue-3682 branch July 19, 2023 14:46
@shuchu shuchu changed the title Fix: issue-4682: run store.plan() only when we need it. Fix: issue-3682: run store.plan() only when we need it. Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants