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

feat: Added Project object to Feast Objects #4475

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

EXPEbdodla
Copy link
Contributor

@EXPEbdodla EXPEbdodla commented Sep 2, 2024

What this PR does / why we need it:

  1. Based on PR fix: Optimize SQL Registry proto() method #4425 discussion, Added support for Project object for all registry's in feature definitions. Defaults to project in feature_store.yaml if not specified. Syncs projects content in registry (table) from feast_metadata on initialization. Opens up an option to specify multiple projects in single repository (Needs more work).
  2. Added a registry configuration purge_feast_metadata to stop using feast_metadata table because list_project_metadata may be used by clients. Instead of deleting that, added a configuration to stop populating table and switch to projects specific methods.
  3. Refactored registry methods _prepare_registry_for_changes and _get_registry_proto methods in registry.py. Refactored proto() methods in sql.py and snowflake.py.
  4. Corrected RegistryConfig model. new() method in registry.py is blocking RegistryConfig inheritance. Removed it and corrected the tests accordingly. Now we can defined configs specific to registry instead of defining all at RegistryConfig class.

Which issue(s) this PR fixes:

Misc

Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>

Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>
@EXPEbdodla EXPEbdodla marked this pull request as ready for review September 3, 2024 05:46
@@ -477,6 +477,16 @@ def __init__(self, name, project=None):
super().__init__(f"Permission {name} does not exist")


class ProjectNotFoundException(Exception):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are both of these necessary? what's the difference. btw, All Feast exceptoins should extend FeastError.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tokoko I noticed the same while doing the latest refactory but I didn't want to extend the scope of changes too much. Looks like other duplications are there like PermissionObjectNotFoundException and DataSourceObjectNotFoundException. Can we remove all of them (in a dedicated ticket maybe)?

Copy link
Contributor Author

@EXPEbdodla EXPEbdodla Sep 3, 2024

Choose a reason for hiding this comment

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

I'll extend the FeastError for ProjectNotFoundException.
Reasons for having two exceptions: Just followed the current convention.
ProjectNotFoundException --> Exception thrown when the DELETE calls are made and project is not found.
ProjectObjectNotFoundException --> Exception thrown when GET calls are made and project is not found. _get_object method passes two args (name, project) when object is not found. I don't want to add another if else logic to filter out project.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dmartinol yeah, this just looks weird. Probably best to defer removal until after this is merged, though.. just to avoid a messy rebase

Copy link
Contributor

Choose a reason for hiding this comment

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

in case, remember an item in the next Release Notes to remind clients to replace any try/except that may have used those duplicate errors.

@tokoko
Copy link
Collaborator

tokoko commented Sep 3, 2024

FYI @dmartinol

@EXPEbdodla
Copy link
Contributor Author

Fixed Integration tests. Removed test_remote_online_store.py apply step because Client Store Registry Apply brings back the server and client registry store states to the contents in the repository (Repository missing Permissions. Permissions are applied through store.apply() method). Seems like bug in previous versions which let it work. Did some refactoring to registry so it's showing up as error now.

Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>
Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>
@EXPEbdodla
Copy link
Contributor Author

@tokoko Please take a look at this PR when you get a chance.

@tokoko
Copy link
Collaborator

tokoko commented Sep 5, 2024

looks good, can you also add usage example in quickstart, just an example of creating a project and specifying some tags.

Signed-off-by: Bhargav Dodla <bdodla@expediagroup.com>
@EXPEbdodla
Copy link
Contributor Author

looks good, can you also add usage example in quickstart, just an example of creating a project and specifying some tags.

Updated quick start document and feast init functionality to include Project object.

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, thanks

@tokoko tokoko merged commit 4a6b663 into feast-dev:master Sep 6, 2024
17 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.

3 participants