Add Discovery API for listing Features #797

terryyylim · 2020-06-13T03:13:49Z

What this PR does / why we need it:
Adds listing functionality for Features.
Features

Can list by project, entities, labels

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

User can now list Features.

core/src/main/java/feast/core/grpc/CoreServiceImpl.java

woop · 2020-06-15T01:07:54Z

core/src/main/java/feast/core/grpc/CoreServiceImpl.java

+      ListEntitiesResponse response = specService.listEntities(request.getFilter());
+      responseObserver.onNext(response);
+      responseObserver.onCompleted();
+    } catch (RetrievalException | IllegalArgumentException | InvalidProtocolBufferException e) {


Is it possible for you to return different gRPC exceptions here instead of INTERNAL for everything? Also, we should probably also catch and return unknown exceptions for the time being, because it can be hard to debug otherwise.

woop · 2020-06-15T01:15:40Z

core/src/main/java/feast/core/model/FeatureSet.java

+  public boolean hasAllEntities(List<String> entitiesFilter) {
+    List<String> allEntitiesName =
+        this.entities.stream()
+            .map(entitySpec -> entitySpec.toProto().getName())


Why are you converting it to a proto?

woop · 2020-06-15T01:17:24Z

core/src/main/java/feast/core/model/FeatureSet.java

+        this.entities.stream()
+            .map(entitySpec -> entitySpec.toProto().getName())
+            .collect(Collectors.toList());
+    for (String entity : entitiesFilter) {


I think it would be easier to sort and compare the lists for equality.

woop · 2020-06-15T01:19:55Z

core/src/main/java/feast/core/model/FeatureSet.java

+              .collect(Collectors.toList());
+      for (Feature feature : validFeatures) {
+        validFeaturesMap.put(
+            this.getProject().getName() + "/" + this.getName() + ":" + feature.getName(), feature);


Isn't this feature reference building available elsewhere in the code base?

woop · 2020-06-15T01:22:14Z

core/src/main/java/feast/core/model/FeatureSet.java

+   * @param labelsFilter contain labels that should be attached to FeatureSet's features
+   * @return Map of Feature references and Features
+   */
+  public Map<String, Feature> getFeaturesByAllLabels(Map<String, String> labelsFilter) {


It seems strange that filtering is a part of this method. Can that be done externally with a static method? I would rather this be getFeaturesByRef and then have a filtering step outside.

woop · 2020-06-15T01:24:59Z

core/src/main/java/feast/core/service/SpecService.java

+   *     filter
+   */
+  public ListFeaturesResponse listFeatures(ListFeaturesRequest.Filter filter)
+      throws InvalidProtocolBufferException {


Where is InvalidProtocolBufferException being thrown and why do you leave it unhandled? It seems like we can add more context there.

woop · 2020-06-15T01:26:06Z

core/src/main/java/feast/core/service/SpecService.java

+      // Matching a specific project
+      featureSets =
+          featureSetRepository.findAllByNameLikeAndProject_NameOrderByNameAsc("%", project);
+    } else {


In what scenario would this else be reached?

woop · 2020-06-15T01:27:09Z

core/src/main/java/feast/core/service/SpecService.java

+    }
+
+    ListFeaturesResponse.Builder response = ListFeaturesResponse.newBuilder();
+    if (featureSets.size() > 0) {


I don't think we need this conditional for size (I know it was in the other method)

woop · 2020-06-15T01:53:30Z

sdk/python/feast/client.py

@@ -493,17 +497,70 @@ def get_feature_set(
            raise grpc.RpcError(e.details())
        return FeatureSet.from_proto(get_feature_set_response.feature_set)

-    def list_entities(self) -> Dict[str, Entity]:
+    def list_features(


Can you please add minimal examples to these two methods?

https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

woop · 2020-06-15T01:54:09Z

sdk/python/feast/client.py

        """
-        Returns a dictionary of entities across all feature sets
+        Returns a list of features based on filters provided


You are missing arguments in your docstring.

woop · 2020-06-15T01:54:21Z

sdk/python/feast/client.py

+
+        return features_dict
+
+    def list_entities(self, project: str = None) -> Dict[str, Entity]:


Missing argument in docstring.

woop · 2020-06-15T01:55:47Z

sdk/python/tests/test_client.py

+        "mocked_client",
+        [pytest.lazy_fixture("mock_client"), pytest.lazy_fixture("secure_mock_client")],
+    )
+    def test_list_entities(self, mocked_client, mocker):


What part of the code are you testing here? Seems like you are testing almost no code.

woop · 2020-06-15T01:57:51Z

tests/e2e/redis/basic-ingest-redis-serving.py

+        ("driver_id", driver_entity)
+    ])
+
+    filter_by_project_entity_labels_expected = OrderedDict([


I dont see a good reason for using OrderedDicts here.

woop

Not too far off the mark, but needs some changes. I would also recommend that you update the relevant documentation for these methods in our documentation.

terryyylim · 2020-06-15T10:19:07Z

/test test-end-to-end-batch

feast-ci-bot · 2020-06-17T14:41:15Z

[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:

~~OWNERS~~ [woop]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

woop · 2020-06-17T14:41:25Z

/lgtm

terryyylim · 2020-06-17T15:05:36Z

/test test-end-to-end-batch

woop · 2020-06-18T03:49:27Z

/lgtm

Terence added 4 commits June 13, 2020 11:08

Add feature and entity listing in feast core

817da94

Update Core protos

5d562d8

Update Python SDK

123a53e

Add tests for new listing functionality

b005578

terryyylim requested review from davidheryanto, khorshuheng, pyalex, woop and zhilingc as code owners June 13, 2020 03:13

feast-ci-bot added needs-kind size/XL labels Jun 13, 2020

Fix python linting

9ec6b15

terryyylim added the kind/feature New feature or request label Jun 13, 2020

feast-ci-bot removed the needs-kind label Jun 13, 2020

Fix python linting

4d15711

terryyylim changed the title ~~Add Discovery API for listing Features and Entities~~ [WIP] Add Discovery API for listing Features and Entities Jun 13, 2020

feast-ci-bot added the do-not-merge/work-in-progress label Jun 13, 2020

Terence added 2 commits June 13, 2020 13:08

Fix e2e redis test

56033e9

Remove check that relies on other feature set

0821ca5

terryyylim changed the title ~~[WIP] Add Discovery API for listing Features and Entities~~ Add Discovery API for listing Features and Entities Jun 13, 2020

feast-ci-bot removed the do-not-merge/work-in-progress label Jun 13, 2020

woop reviewed Jun 15, 2020

View reviewed changes

core/src/main/java/feast/core/grpc/CoreServiceImpl.java Show resolved Hide resolved

woop reviewed Jun 15, 2020

View reviewed changes

woop requested changes Jun 15, 2020

View reviewed changes

Address PR comments

d5e2fc7

terryyylim force-pushed the discovery-api branch from c85cae3 to d5e2fc7 Compare June 15, 2020 06:09

Address more PR comments

4fbd94c

terryyylim force-pushed the discovery-api branch from 2d33fe9 to 4fbd94c Compare June 15, 2020 07:32

Revert ListEntities implementation

50c2e54

terryyylim changed the title ~~Add Discovery API for listing Features and Entities~~ Add Discovery API for listing Features Jun 15, 2020

ches added this to the v0.6.0 milestone Jun 16, 2020

Terence and others added 4 commits June 17, 2020 19:51

Merge branch 'master' into discovery-api

5171d76

Apply spotless

44962b8

Add description for ListFeaturesRequest proto

e22d8c7

Fix comment

6a13aab

woop approved these changes Jun 17, 2020

View reviewed changes

feast-ci-bot added the approved label Jun 17, 2020

feast-ci-bot assigned woop Jun 17, 2020

feast-ci-bot added the lgtm label Jun 17, 2020

Apply spotless

497508f

feast-ci-bot removed the lgtm label Jun 17, 2020

feast-ci-bot added the lgtm label Jun 18, 2020

feast-ci-bot merged commit e343c2f into feast-dev:master Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Discovery API for listing Features #797

Add Discovery API for listing Features #797

terryyylim commented Jun 13, 2020 •

edited

Loading

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020

woop Jun 15, 2020 •

edited

Loading

woop Jun 15, 2020 •

edited

Loading

woop Jun 15, 2020

woop left a comment

terryyylim commented Jun 15, 2020

feast-ci-bot commented Jun 17, 2020

woop commented Jun 17, 2020

terryyylim commented Jun 17, 2020

woop commented Jun 18, 2020


		return features_dict

		def list_entities(self, project: str = None) -> Dict[str, Entity]:

Add Discovery API for listing Features #797

Add Discovery API for listing Features #797

Conversation

terryyylim commented Jun 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woop Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

woop Jun 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woop left a comment

Choose a reason for hiding this comment

terryyylim commented Jun 15, 2020

feast-ci-bot commented Jun 17, 2020

woop commented Jun 17, 2020

terryyylim commented Jun 17, 2020

woop commented Jun 18, 2020

terryyylim commented Jun 13, 2020 •

edited

Loading

woop Jun 15, 2020 •

edited

Loading

woop Jun 15, 2020 •

edited

Loading