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: Check for snowflake functions when setting up materialization engine #4456

Merged
merged 2 commits into from
Aug 30, 2024

Conversation

benchoncy
Copy link
Contributor

What this PR does / why we need it:

When running feast apply using the snowflake materialization engine, feast only checks if the stage in snowflake exists and assumes the functions have been created. This allows a case where if anything goes wrong during function creation, such as permission issues, a stage can be created while the functions are not. Following attempts to feast apply see the created stage and tell the user that the functions exist without creating them. This leads to materialization on snowflake to fail down the line due to missing functions.

This PR aims to change the apply logic to instead check for snowflake functions before deciding if action is required.

Which issue(s) this PR fixes:

Misc

A current workaround for this is to delete the stage in Snowflake before running the next feast apply.

Signed-off-by: Ben Stuart <ben@benstuart.ie>
Signed-off-by: Ben Stuart <ben@benstuart.ie>
@tokoko
Copy link
Collaborator

tokoko commented Aug 29, 2024

Why remove the existing stage check? Does checking for functions make checking for stage redundant?

@benchoncy
Copy link
Contributor Author

Why remove the existing stage check? Does checking for functions make checking for stage redundant?

Thanks for the review, @tokoko. My understanding is the stage is a stepping stone to creating the functions as a place to upload and store files required by the functions. The main objective is to create the functions, as part of that feast creates a stage, but outside of these functions, this stage goes unused.

The functions cannot exist without the stage, and the stage is not reused elsewhere, so a stage check is redundant when checking for functions. Instead, I've added IF NOT EXISTS to the stage creation to make the operation idempotent and avoid failure if the stage already exists. I hope this better explains my reasoning.

@tokoko
Copy link
Collaborator

tokoko commented Aug 30, 2024

thanks, makes sense. I guess this still doesn't account for the changes in function definitions themselves, right? If we add some function to be deployed, a user will have to remove the stage or all functions for them to be correctly recreated. A problem for another day...

Copy link
Collaborator

@tokoko tokoko left a comment

Choose a reason for hiding this comment

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

lgtm

@tokoko tokoko merged commit c365b4e into feast-dev:master Aug 30, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants