Introduce Entity as higher-level concept #1014
Conversation
terryyylim
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
protos/feast/core/CoreService.proto
Outdated
| message ApplyEntityRequest { | ||
| // If project is unspecified, will default to 'default' project. | ||
| // If project specified does not exist, the project would be automatically created. | ||
| feast.core.Entity entity = 1; |
There was a problem hiding this comment.
Which parts of the entity can be updated?
There was a problem hiding this comment.
That can be found here.
There was a problem hiding this comment.
Does it make more sense to expose only EntitySpecV2 instead to users, as the other (meta) fields are set by Feast?
There was a problem hiding this comment.
agreed, EntitySpecV2 should be here instead
|
Make sure to update go language definitions your new protobufs with |
| * @param filter Filter containing the desired entity name, project and labels | ||
| * @return ListEntitiesResponse with list of entities found matching the filter | ||
| */ | ||
| public ListEntitiesResponse listEntities(ListEntitiesRequest.Filter filter) { |
There was a problem hiding this comment.
Do we have a strong use for this wildcard filters or can we remove it?
There was a problem hiding this comment.
I guess it'll be easier to locate a set of entities that are registered with a certain pattern within a project.
There was a problem hiding this comment.
Not a strong need, have removed it.
There was a problem hiding this comment.
Hmmm, what have you removed? I still see name.replace('*', '%'), project.replace('*', '%'))
There was a problem hiding this comment.
If its easier to keep in then we can keep it in, but this seems like a lot of complexity to keep if we dont actually need it.
There was a problem hiding this comment.
Oh, I disallowed searching for wildcard names in a specific project. So if we're removing wildcard filters, should we update the proto to only consider project and labels in the Filter then?
There was a problem hiding this comment.
Doesn't seem very useful to have entity name in the filter since it'll just return that entity in a list. The user might be better off using getEntity in this case.
core/src/main/resources/db/migration/V2.7__Entities_Higher_Level_Concept.sql
Show resolved
Hide resolved
core/src/main/resources/db/migration/V2.7__Entities_Higher_Level_Concept.sql
Show resolved
Hide resolved
|
Doesnt this PR introduce a user facing change? |
terryyylim
changed the title
terryyylim
changed the title
| @@ -44,3 +55,273 @@ def from_proto(cls, entity_proto: EntityProto): | |||
| """ | |||
| entity = cls(name=entity_proto.name, dtype=ValueType(entity_proto.value_type)) | |||
| return entity | |||
|
|
|||
|
|
|||
| class EntityV2: | |||
There was a problem hiding this comment.
Do we really need to expose the V2 to users? I think we can replace the existing Entity with this new one. Any concerns with that @mrzzy ?
There was a problem hiding this comment.
I think that's okay, but we'll need to update (perhaps use another name?) the previous Entity implementation used in tests and feature set. However, this would introduce breaking change to existing users who are still using the featureset-entity concept.
eg. if they do something like
fs = FeatureSet(
"featureset",
features=[
Feature("feat1", ValueType.STRING)
],
entities=[Entity("entity_id", ValueType.INT64)],
max_age=Duration(seconds=100),
)
There was a problem hiding this comment.
Will be updating SDK in separate PR, along with replacing existing e2e tests.
|
Looks good to me. I will approve. @pyalex can you lgtm when you are happy. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: terryyylim, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
What this PR does / why we need it:
Introducing entities as a higher-level concept is the first step in generalizing Feast. The Feast v0.8 proposal which seeks to do so can be found here.
This PR does not include versioning of protos nor removal of existing EntitySpec concept. This will be done in a separate PR in the near future.
Which issue(s) this PR fixes:
Fixes #405
Does this PR introduce a user-facing change?: