feat: Added Project object to Feast Objects by EXPEbdodla · Pull Request #4475 · feast-dev/feast
What this PR does / why we need it:
- 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).
- Added a registry configuration
purge_feast_metadatato stop usingfeast_metadatatable 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. - Refactored registry methods
_prepare_registry_for_changesand_get_registry_protomethods in registry.py. Refactored proto() methods in sql.py and snowflake.py. - 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>
| super().__init__(f"Permission {name} does not exist") | ||
|
|
||
|
|
||
| class ProjectNotFoundException(Exception): |
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.
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)?
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.
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
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.
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>
@tokoko Please take a look at this PR when you get a chance.
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>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks
lokeshrangineni pushed a commit to lokeshrangineni/feast that referenced this pull request
lokeshrangineni pushed a commit to lokeshrangineni/feast that referenced this pull request