From 77a83fd90d24b769a54d275f6ea4dcafa47754ac Mon Sep 17 00:00:00 2001 From: Terence Lim Date: Fri, 20 Nov 2020 16:55:45 +0800 Subject: [PATCH] Update Feast Core list features method (#1176) * Update listFeatures method in core Signed-off-by: Terence * Update tests Signed-off-by: Terence * Address PR comments Signed-off-by: Terence * Update python SDK Signed-off-by: Terence * Make integration test clearer Signed-off-by: Terence --- .../feast/common/it/SimpleCoreClient.java | 4 +- .../controller/CoreServiceRestController.java | 2 +- .../java/feast/core/model/FeatureTable.java | 71 ++++++++++++++++- .../main/java/feast/core/model/FeatureV2.java | 17 ++++ .../java/feast/core/service/SpecService.java | 57 ++++++------- .../core/controller/CoreServiceRestIT.java | 18 ++--- .../feast/core/service/SpecServiceIT.java | 79 +++++++++++++------ protos/feast/core/CoreService.proto | 5 +- sdk/python/feast/client.py | 49 +++++++++++- sdk/python/feast/feature.py | 9 +++ sdk/python/tests/test_client.py | 40 ++++++++++ 11 files changed, 274 insertions(+), 77 deletions(-) diff --git a/common-test/src/main/java/feast/common/it/SimpleCoreClient.java b/common-test/src/main/java/feast/common/it/SimpleCoreClient.java index cffb4f42af..3c3bfbc553 100644 --- a/common-test/src/main/java/feast/common/it/SimpleCoreClient.java +++ b/common-test/src/main/java/feast/common/it/SimpleCoreClient.java @@ -162,7 +162,7 @@ public void updateFeatureSetStatus( .build()); } - public Map simpleListFeatures( + public Map simpleListFeatures( String projectName, Map labels, List entities) { return stub.listFeatures( CoreServiceProto.ListFeaturesRequest.newBuilder() @@ -176,7 +176,7 @@ public Map simpleListFeatures( .getFeaturesMap(); } - public Map simpleListFeatures( + public Map simpleListFeatures( String projectName, String... entities) { return simpleListFeatures(projectName, Collections.emptyMap(), Arrays.asList(entities)); } diff --git a/core/src/main/java/feast/core/controller/CoreServiceRestController.java b/core/src/main/java/feast/core/controller/CoreServiceRestController.java index 7d661e6475..0549a536cf 100644 --- a/core/src/main/java/feast/core/controller/CoreServiceRestController.java +++ b/core/src/main/java/feast/core/controller/CoreServiceRestController.java @@ -114,7 +114,7 @@ public ListFeatureSetsResponse listFeatureSets( * default. * @return (200 OK) Return {@link ListFeaturesResponse} in JSON. */ - @RequestMapping(value = "/v1/features", method = RequestMethod.GET) + @RequestMapping(value = "/v2/features", method = RequestMethod.GET) public ListFeaturesResponse listFeatures( @RequestParam String[] entities, @RequestParam(required = false) Optional project) { ListFeaturesRequest.Filter.Builder filterBuilder = diff --git a/core/src/main/java/feast/core/model/FeatureTable.java b/core/src/main/java/feast/core/model/FeatureTable.java index 8c62df6486..19515faadf 100644 --- a/core/src/main/java/feast/core/model/FeatureTable.java +++ b/core/src/main/java/feast/core/model/FeatureTable.java @@ -16,6 +16,8 @@ */ package feast.core.model; +import static feast.common.models.FeatureV2.getFeatureStringRef; + import com.google.common.hash.Hashing; import com.google.protobuf.Duration; import com.google.protobuf.Timestamp; @@ -25,6 +27,7 @@ import feast.proto.core.FeatureProto.FeatureSpecV2; import feast.proto.core.FeatureTableProto; import feast.proto.core.FeatureTableProto.FeatureTableSpec; +import feast.proto.serving.ServingAPIProto; import java.util.*; import java.util.stream.Collectors; import javax.persistence.CascadeType; @@ -73,7 +76,7 @@ public class FeatureTable extends AbstractTimestampEntity { private Set features; // Entites to associate the features defined in this FeatureTable with - @ManyToMany + @ManyToMany(fetch = FetchType.EAGER) @JoinTable( name = "feature_tables_entities_v2", joinColumns = @JoinColumn(name = "feature_table_id"), @@ -263,6 +266,72 @@ private static Set resolveEntities( .collect(Collectors.toSet()); } + /** + * Return a boolean to indicate if FeatureTable contains all specified entities. + * + * @param entitiesFilter contain entities that should be attached to the FeatureTable + * @return boolean True if FeatureTable contains all entities in the entitiesFilter + */ + public boolean hasAllEntities(List entitiesFilter) { + Set allEntitiesName = + this.getEntities().stream().map(entity -> entity.getName()).collect(Collectors.toSet()); + return allEntitiesName.equals(new HashSet<>(entitiesFilter)); + } + + /** + * Returns a map of Feature references and Features if FeatureTable's Feature contains all labels + * in the labelsFilter + * + * @param labelsFilter contain labels that should be attached to FeatureTable's features + * @return Map of Feature references and Features + */ + public Map getFeaturesByLabels(Map labelsFilter) { + Map validFeaturesMap; + List validFeatures; + if (labelsFilter.size() > 0) { + validFeatures = filterFeaturesByAllLabels(this.getFeatures(), labelsFilter); + validFeaturesMap = getFeaturesRefToFeaturesMap(validFeatures); + return validFeaturesMap; + } + validFeaturesMap = getFeaturesRefToFeaturesMap(List.copyOf(this.getFeatures())); + return validFeaturesMap; + } + + /** + * Returns map for accessing features using their respective feature reference. + * + * @param features List of features to insert to map. + * @return Map of featureRef:feature. + */ + private Map getFeaturesRefToFeaturesMap(List features) { + Map validFeaturesMap = new HashMap<>(); + for (FeatureV2 feature : features) { + ServingAPIProto.FeatureReferenceV2 featureRef = + ServingAPIProto.FeatureReferenceV2.newBuilder() + .setFeatureTable(this.getName()) + .setName(feature.getName()) + .build(); + validFeaturesMap.put(getFeatureStringRef(featureRef), feature); + } + return validFeaturesMap; + } + + /** + * Returns a list of Features if FeatureTable's Feature contains all labels in labelsFilter + * + * @param labelsFilter contain labels that should be attached to FeatureTable's features + * @return List of Features + */ + public static List filterFeaturesByAllLabels( + Set features, Map labelsFilter) { + List validFeatures = + features.stream() + .filter(feature -> feature.hasAllLabels(labelsFilter)) + .collect(Collectors.toList()); + + return validFeatures; + } + /** * Determine whether a FeatureTable has all the specified labels. * diff --git a/core/src/main/java/feast/core/model/FeatureV2.java b/core/src/main/java/feast/core/model/FeatureV2.java index e10d51647c..f25e951efc 100644 --- a/core/src/main/java/feast/core/model/FeatureV2.java +++ b/core/src/main/java/feast/core/model/FeatureV2.java @@ -106,6 +106,23 @@ public void updateFromProto(FeatureSpecV2 spec) { this.labelsJSON = TypeConversion.convertMapToJsonString(spec.getLabelsMap()); } + /** + * Return a boolean to indicate if Feature contains all specified labels. + * + * @param labelsFilter contain labels that should be attached to Feature + * @return boolean True if Feature contains all labels in the labelsFilter + */ + public boolean hasAllLabels(Map labelsFilter) { + Map featureLabelsMap = TypeConversion.convertJsonStringToMap(getLabelsJSON()); + for (String key : labelsFilter.keySet()) { + if (!featureLabelsMap.containsKey(key) + || !featureLabelsMap.get(key).equals(labelsFilter.get(key))) { + return false; + } + } + return true; + } + @Override public int hashCode() { return Objects.hash(getName(), getType(), getLabelsJSON()); diff --git a/core/src/main/java/feast/core/service/SpecService.java b/core/src/main/java/feast/core/service/SpecService.java index 9f6569fb82..c9ce7d1b72 100644 --- a/core/src/main/java/feast/core/service/SpecService.java +++ b/core/src/main/java/feast/core/service/SpecService.java @@ -297,46 +297,37 @@ public ListFeatureSetsResponse listFeatureSets(ListFeatureSetsRequest.Filter fil * filter */ public ListFeaturesResponse listFeatures(ListFeaturesRequest.Filter filter) { - try { - String project = filter.getProject(); - List entities = filter.getEntitiesList(); - Map labels = filter.getLabelsMap(); + String project = filter.getProject(); + List entities = filter.getEntitiesList(); + Map labels = filter.getLabelsMap(); - checkValidCharactersAllowAsterisk(project, "project"); + checkValidCharactersAllowAsterisk(project, "project"); - // Autofill default project if project not specified - if (project.isEmpty()) { - project = Project.DEFAULT_NAME; - } + // Autofill default project if project not specified + if (project.isEmpty()) { + project = Project.DEFAULT_NAME; + } - // Currently defaults to all FeatureSets - List featureSets = - featureSetRepository.findAllByNameLikeAndProject_NameOrderByNameAsc("%", project); - // TODO: List features in Feature Tables. + // Currently defaults to all FeatureTables + List featureTables = tableRepository.findAllByProject_Name(project); - ListFeaturesResponse.Builder response = ListFeaturesResponse.newBuilder(); - if (entities.size() > 0) { - featureSets = - featureSets.stream() - .filter(featureSet -> featureSet.hasAllEntities(entities)) - .collect(Collectors.toList()); - } + ListFeaturesResponse.Builder response = ListFeaturesResponse.newBuilder(); + if (entities.size() > 0) { + featureTables = + featureTables.stream() + .filter(featureTable -> featureTable.hasAllEntities(entities)) + .collect(Collectors.toList()); + } - Map featuresMap; - for (FeatureSet featureSet : featureSets) { - featuresMap = featureSet.getFeaturesByRef(labels); - for (Map.Entry entry : featuresMap.entrySet()) { - response.putFeatures(entry.getKey(), entry.getValue().toProto()); - } + Map featuresMap; + for (FeatureTable featureTable : featureTables) { + featuresMap = featureTable.getFeaturesByLabels(labels); + for (Map.Entry entry : featuresMap.entrySet()) { + response.putFeatures(entry.getKey(), entry.getValue().toProto()); } - - return response.build(); - } catch (InvalidProtocolBufferException e) { - throw io.grpc.Status.NOT_FOUND - .withDescription("Unable to retrieve features") - .withCause(e) - .asRuntimeException(); } + + return response.build(); } /** diff --git a/core/src/test/java/feast/core/controller/CoreServiceRestIT.java b/core/src/test/java/feast/core/controller/CoreServiceRestIT.java index ea29591849..0297060feb 100644 --- a/core/src/test/java/feast/core/controller/CoreServiceRestIT.java +++ b/core/src/test/java/feast/core/controller/CoreServiceRestIT.java @@ -176,12 +176,9 @@ public void listFeatureSets() { @Test public void listFeatures() { - // entities = [merchant_id] - // project = default - // should return 4 features String uri1 = - UriComponentsBuilder.fromPath("/api/v1/features") - .queryParam("entities", "merchant_id") + UriComponentsBuilder.fromPath("/api/v2/features") + .queryParam("entities", "entity1", "entity2") .buildAndExpand() .toString(); get(uri1) @@ -190,15 +187,12 @@ public void listFeatures() { .everything() .assertThat() .contentType(ContentType.JSON) - .body("features", aMapWithSize(4)); + .body("features", aMapWithSize(2)); - // entities = [merchant_id] - // project = merchant - // should return 2 features String uri2 = - UriComponentsBuilder.fromPath("/api/v1/features") - .queryParam("entities", "merchant_id") - .queryParam("project", "merchant") + UriComponentsBuilder.fromPath("/api/v2/features") + .queryParam("entities", "entity1", "entity2") + .queryParam("project", "default") .buildAndExpand() .toString(); get(uri2) diff --git a/core/src/test/java/feast/core/service/SpecServiceIT.java b/core/src/test/java/feast/core/service/SpecServiceIT.java index 8d56de606b..7f0b88539c 100644 --- a/core/src/test/java/feast/core/service/SpecServiceIT.java +++ b/core/src/test/java/feast/core/service/SpecServiceIT.java @@ -102,6 +102,23 @@ public void initState() { .setBatchSource( DataGenerator.createFileDataSourceSpec("file:///path/to/file", "ts_col", "")) .build()); + apiClient.applyFeatureTable( + "default", + DataGenerator.createFeatureTableSpec( + "featuretable2", + Arrays.asList("entity1", "entity2"), + new HashMap<>() { + { + put("feature3", ValueProto.ValueType.Enum.STRING); + put("feature4", ValueProto.ValueType.Enum.FLOAT); + } + }, + 7200, + ImmutableMap.of("feat_key4", "feat_value4")) + .toBuilder() + .setBatchSource( + DataGenerator.createFileDataSourceSpec("file:///path/to/file", "ts_col", "")) + .build()); apiClient.simpleApplyEntity( "project1", DataGenerator.createEntitySpecV2( @@ -312,10 +329,13 @@ public void shouldUseDefaultProjectIfProjectUnspecified() { List featureTables = apiClient.simpleListFeatureTables(filter); - assertThat(featureTables, hasSize(1)); + assertThat(featureTables, hasSize(2)); assertThat( featureTables, hasItem(hasProperty("spec", hasProperty("name", equalTo("featuretable1"))))); + assertThat( + featureTables, + hasItem(hasProperty("spec", hasProperty("name", equalTo("featuretable2"))))); } @Test @@ -1005,49 +1025,55 @@ class ListFeatures { @Test public void shouldFilterFeaturesByEntitiesAndLabels() { // Case 1: Only filter by entities - Map result1 = - apiClient.simpleListFeatures("project1", "user_id"); + Map result1 = + apiClient.simpleListFeatures("default", "entity1", "entity2"); - assertThat(result1, aMapWithSize(2)); - assertThat(result1, hasKey(equalTo("project1/fs3:feature1"))); - assertThat(result1, hasKey(equalTo("project1/fs3:feature2"))); + assertThat(result1, aMapWithSize(4)); + assertThat(result1, hasKey(equalTo("featuretable1:feature1"))); + assertThat(result1, hasKey(equalTo("featuretable1:feature2"))); + assertThat(result1, hasKey(equalTo("featuretable2:feature3"))); + assertThat(result1, hasKey(equalTo("featuretable2:feature4"))); // Case 2: Filter by entities and labels - Map result2 = + Map result2 = apiClient.simpleListFeatures( - "project1", - ImmutableMap.of("app", "feast", "version", "one"), - ImmutableList.of("customer_id")); + "default", + ImmutableMap.of("feat_key2", "feat_value2"), + ImmutableList.of("entity1", "entity2")); - assertThat(result2, aMapWithSize(1)); - assertThat(result2, hasKey(equalTo("project1/fs4:feature2"))); + assertThat(result2, aMapWithSize(2)); + assertThat(result2, hasKey(equalTo("featuretable1:feature1"))); + assertThat(result2, hasKey(equalTo("featuretable1:feature2"))); // Case 3: Filter by labels - Map result3 = + Map result3 = apiClient.simpleListFeatures( - "project1", ImmutableMap.of("app", "feast"), Collections.emptyList()); + "default", ImmutableMap.of("feat_key4", "feat_value4"), Collections.emptyList()); assertThat(result3, aMapWithSize(2)); - assertThat(result3, hasKey(equalTo("project1/fs4:feature2"))); - assertThat(result3, hasKey(equalTo("project1/fs5:feature3"))); + assertThat(result3, hasKey(equalTo("featuretable2:feature3"))); + assertThat(result3, hasKey(equalTo("featuretable2:feature4"))); // Case 4: Filter by nothing, except project - Map result4 = + Map result4 = apiClient.simpleListFeatures("project1", ImmutableMap.of(), Collections.emptyList()); - assertThat(result4, aMapWithSize(4)); - assertThat(result4, hasKey(equalTo("project1/fs3:feature1"))); - assertThat(result4, hasKey(equalTo("project1/fs3:feature1"))); - assertThat(result4, hasKey(equalTo("project1/fs4:feature2"))); - assertThat(result4, hasKey(equalTo("project1/fs5:feature3"))); + assertThat(result4, aMapWithSize(0)); // Case 5: Filter by nothing; will use default project - Map result5 = + Map result5 = apiClient.simpleListFeatures("", ImmutableMap.of(), Collections.emptyList()); - assertThat(result5, aMapWithSize(2)); - assertThat(result5, hasKey(equalTo("default/fs1:total"))); - assertThat(result5, hasKey(equalTo("default/fs2:sum"))); + assertThat(result5, aMapWithSize(4)); + assertThat(result5, hasKey(equalTo("featuretable1:feature1"))); + assertThat(result5, hasKey(equalTo("featuretable1:feature2"))); + assertThat(result5, hasKey(equalTo("featuretable2:feature3"))); + assertThat(result5, hasKey(equalTo("featuretable2:feature4"))); + + // Case 6: Filter by mismatched entity + Map result6 = + apiClient.simpleListFeatures("default", ImmutableMap.of(), ImmutableList.of("entity1")); + assertThat(result6, aMapWithSize(0)); } } @@ -1350,6 +1376,7 @@ public void shouldReturnNoTables() { CoreServiceProto.ListFeatureTablesRequest.Filter filter = CoreServiceProto.ListFeatureTablesRequest.Filter.newBuilder() .setProject("default") + .putLabels("feat_key2", "feat_value2") .build(); List featureTables = apiClient.simpleListFeatureTables(filter); diff --git a/protos/feast/core/CoreService.proto b/protos/feast/core/CoreService.proto index 92abe1dc19..8eb13514b4 100644 --- a/protos/feast/core/CoreService.proto +++ b/protos/feast/core/CoreService.proto @@ -24,6 +24,7 @@ option java_package = "feast.proto.core"; import "google/protobuf/timestamp.proto"; import "tensorflow_metadata/proto/v0/statistics.proto"; import "feast/core/Entity.proto"; +import "feast/core/Feature.proto"; import "feast/core/FeatureSet.proto"; import "feast/core/FeatureTable.proto"; import "feast/core/Store.proto"; @@ -238,7 +239,9 @@ message ListFeaturesRequest { } message ListFeaturesResponse { - map features = 1; + reserved 1; + + map features = 2; } message ListStoresRequest { diff --git a/sdk/python/feast/client.py b/sdk/python/feast/client.py index 231230a155..215ac8cc32 100644 --- a/sdk/python/feast/client.py +++ b/sdk/python/feast/client.py @@ -42,6 +42,8 @@ GetFeatureTableResponse, ListEntitiesRequest, ListEntitiesResponse, + ListFeaturesRequest, + ListFeaturesResponse, ListFeatureTablesRequest, ListFeatureTablesResponse, ListProjectsRequest, @@ -59,7 +61,7 @@ from feast.data_format import ParquetFormat from feast.data_source import BigQuerySource, FileSource from feast.entity import Entity -from feast.feature import _build_feature_references +from feast.feature import Feature, FeatureRef, _build_feature_references from feast.feature_table import FeatureTable from feast.grpc import auth as feast_auth from feast.grpc.grpc import create_grpc_channel @@ -693,6 +695,51 @@ def delete_feature_table(self, name: str, project: str = None) -> None: except grpc.RpcError as e: raise grpc.RpcError(e.details()) + def list_features_by_ref( + self, + project: str = None, + entities: List[str] = list(), + labels: Dict[str, str] = dict(), + ) -> Dict[FeatureRef, Feature]: + """ + Retrieve a dictionary of feature reference to feature from Feast Core based on filters provided. + + Args: + project: Feast project that these features belongs to + entities: Feast entity that these features are associated with + labels: Feast labels that these features are associated with + + Returns: + Dictionary of + + Examples: + >>> from feast import Client + >>> + >>> feast_client = Client(core_url="localhost:6565") + >>> features = feast_client.list_features(project="test_project", entities=["driver_id"], labels={"key1":"val1","key2":"val2"}) + >>> print(features) + """ + + if project is None: + project = self.project + + filter = ListFeaturesRequest.Filter( + project=project, entities=entities, labels=labels + ) + + feature_protos = self._core_service.ListFeatures( + ListFeaturesRequest(filter=filter), metadata=self._get_grpc_metadata(), + ) # type: ListFeaturesResponse + + # Extract features and return + features_dict = {} + for ref_str, feature_proto in feature_protos.features.items(): + feature_ref = FeatureRef.from_str(ref_str) + feature = Feature.from_proto(feature_proto) + features_dict[feature_ref] = feature + + return features_dict + def ingest( self, feature_table: Union[str, FeatureTable], diff --git a/sdk/python/feast/feature.py b/sdk/python/feast/feature.py index 1d0e525a89..16c8ca57ee 100644 --- a/sdk/python/feast/feature.py +++ b/sdk/python/feast/feature.py @@ -148,6 +148,15 @@ def to_proto(self) -> FeatureRefProto: return self.proto + def __repr__(self): + # return string representation of the reference + ref_str = self.proto.feature_table + ":" + self.proto.name + return ref_str + + def __str__(self): + # readable string of the reference + return f"FeatureRef<{self.__repr__()}>" + def _build_feature_references(feature_ref_strs: List[str]) -> List[FeatureRefProto]: """ diff --git a/sdk/python/tests/test_client.py b/sdk/python/tests/test_client.py index 564a7b671c..8281641ede 100644 --- a/sdk/python/tests/test_client.py +++ b/sdk/python/tests/test_client.py @@ -35,6 +35,7 @@ from feast.core.CoreService_pb2 import ( GetFeastCoreVersionResponse, GetFeatureTableResponse, + ListFeaturesResponse, ) from feast.core.DataSource_pb2 import DataSource as DataSourceProto from feast.core.Feature_pb2 import FeatureSpecV2 as FeatureSpecProto @@ -447,6 +448,45 @@ def test_apply_feature_table_success(self, test_client): and feature_tables[0].entities[0] == "fs1-my-entity-1" ) + @pytest.mark.parametrize( + "test_client", [lazy_fixture("client"), lazy_fixture("secure_client")] + ) + def test_list_features(self, test_client, mocker): + mocker.patch.object( + test_client, + "_core_service_stub", + return_value=Core.CoreServiceStub(grpc.insecure_channel("")), + ) + + feature1_proto = FeatureSpecProto( + name="feature_1", value_type=ValueProto.ValueType.FLOAT + ) + feature2_proto = FeatureSpecProto( + name="feature_2", value_type=ValueProto.ValueType.STRING + ) + + mocker.patch.object( + test_client._core_service_stub, + "ListFeatures", + return_value=ListFeaturesResponse( + features={ + "driver_car:feature_1": feature1_proto, + "driver_car:feature_2": feature2_proto, + } + ), + ) + + features = test_client.list_features_by_ref(project="test") + assert len(features) == 2 + + native_feature_list = [] + for _, feature_proto in features.items(): + native_feature_list.append(feature_proto) + + assert sorted(native_feature_list) == sorted( + [Feature.from_proto(feature1_proto), Feature.from_proto(feature2_proto)] + ) + @pytest.mark.parametrize( "mocked_client", [lazy_fixture("mock_client")], )