feat: Add registry methods for dealing with all FV types by tokoko · Pull Request #4435 · feast-dev/feast
Signed-off-by: tokoko <togurgenidze@gmail.com>
Signed-off-by: tokoko <togurgenidze@gmail.com>
| def GetAnyFeatureView( | ||
| self, request: RegistryServer_pb2.GetAnyFeatureViewRequest, context | ||
| ): | ||
| feature_view = assert_permissions( |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking PR
| bool commit = 3; | ||
| } | ||
|
|
||
| message AnyFeatureView { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why AnyFeatureView and not AllFeatureViews?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is a single message though so I see.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't we call it FeatureViewType?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like type is something else, it would refer to one the types, not one of the objects. AnyFeatureView basically stands for AnyTypeOfFeatureView.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see what you're doing this for.
Man this is ugly.
The get_any_feature_view should probably be an intermediate solution and instead we should find a way to make get_feature_view include ODFV and Stream FVs.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get why we can't jump right to that, but maybe we should start a doc on what should be reconciled for release to Feast 1.0?
| self.commit() | ||
| return | ||
|
|
||
| raise FeatureViewNotFoundException(feature_view.name, project) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to drop this from apply_materialization?
|
|
||
| all_feature_views = test_registry.list_all_feature_views(project, tags=sfv.tags) | ||
|
|
||
| print(stream_feature_views) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove these print statements now
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this is great, thanks Tornike!
As mentioned in a comment, I think we should start jotting down some major clean ups we want to do to release 1.0. I'm experiencing this as I'm working through the writes to the ODFV and I think there's a way for us to clean up a lot more.
There's a lot of code in between class instantiation and apply that is very opaque and makes it hard to reason about what's happening. It's for reasonable choices but I think it warrants a broad review of the online store.
Also, it could be a good time to establish DuckDB as the offline store default.
As mentioned in a comment, I think we should start jotting down some major clean ups we want to do to release 1.0. I'm experiencing this as I'm working through the writes to the ODFV and I think there's a way for us to clean up a lot more.
Yeah, there's probably a lot of discussion to be had until we finalize these APIs for 1.0 release. We should definitely continue the debate in #4235. The problem with get_feature_view is that it might be interpreted as referring to all feature views and also to plain FeatureView objects specifically.
Also, it could be a good time to establish DuckDB as the offline store default.
Yeah, that's probably a pretty good idea. I recently had to make a simple demo and saw that on small datasets duckdb was a lot faster than the current default, dask. I'll open a ticket for this and let's continue there, namely I think we should agree whether duckdb being the default means that it should be a required dependency or not (I think not...)
Signed-off-by: tokoko <togurgenidze@gmail.com>
Signed-off-by: tokoko <togurgenidze@gmail.com>
As mentioned in a comment, I think we should start jotting down some major clean ups we want to do to release 1.0. I'm experiencing this as I'm working through the writes to the ODFV and I think there's a way for us to clean up a lot more.
Yeah, there's probably a lot of discussion to be had until we finalize these APIs for 1.0 release. We should definitely continue the debate in #4235. The problem with
get_feature_viewis that it might be interpreted as referring to all feature views and also to plainFeatureViewobjects specifically.Also, it could be a good time to establish DuckDB as the offline store default.
Yeah, that's probably a pretty good idea. I recently had to make a simple demo and saw that on small datasets duckdb was a lot faster than the current default, dask. I'll open a ticket for this and let's continue there, namely I think we should agree whether duckdb being the default means that it should be a required dependency or not (I think not...)
I think a default offline store should be required, so in that case we shouldn't make it the default.
What are your thoughts about us drafting a 1.0 Roadmap? It's September now, so a rational question would be: could we get it to 1.0 by end of the year? I'm not sure (probably not).
lokeshrangineni pushed a commit to lokeshrangineni/feast that referenced this pull request
lokeshrangineni pushed a commit to lokeshrangineni/feast that referenced this pull request