Labels for feature by imjuanleonard · Pull Request #499 · feast-dev/feast

imjuanleonard · 2020-02-27T03:24:05Z

Ensure that your code follows our code conventions ✅
Run unit tests and ensure that they are passing ✅
If your change introduces any API changes, make sure to update the integration tests scripts here ✅
Make sure documentation is updated for your PR! ✅
Make sure you have signed the CLA https://cla.developers.google.com/clas ✅

What this PR does / why we need it:
Full discussion is available on this issue

Which issue(s) this PR fixes:
Fixes #463 on the service side

Does this PR introduce a user-facing change?:

- Add labels column to Feature/Entity

feast-ci-bot · 2020-02-27T03:24:20Z

Hi @imjuanleonard. Thanks for your PR.

I'm waiting for a gojek member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

woop · 2020-02-27T04:28:23Z

sdk/python/feast/feature_set.py

Label isnt a property of a feature set, its a property of a feature.

My bad, Thanks for clarifying

Hence, this is current to do Checklist:

Add Mac development set-up guide using homebrew

Create the labels column on Field Class (This will affect the Feature and Entity)

Create a gateway to Create, List (Should be able to print the labels column), and Delete to the labels Column in Feature class using python SDK Client

Add the test with labels Column empty and labels Column marked in Feature class

Update the integration test tree

Update the documentation on how to use the labels from Feature function

Notes, Datatype for each of the layers

Python: dict() Java: Map<String, String> Table: Varchar(255): with key-value object store in JSON

Varchar(255): with key-value object store in JSON

Why are you limiting it to that small of a size? I also think you should maybe consider comma or tab separation here and using a Set (in the Java model)

Sure @woop , now I am using 'TEXT' for a result similar with Varchar(Max), thanks for the input

feast-ci-bot · 2020-03-02T07:22:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: imjuanleonard
To complete the pull request process, please assign woop
You can assign the PR to them by writing /assign @woop in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

feast-ci-bot · 2020-03-02T07:22:27Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: imjuanleonard
To complete the pull request process, please assign woop
You can assign the PR to them by writing /assign @woop in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

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

woop · 2020-03-02T07:30:05Z

/ok-to-test

woop · 2020-03-02T09:34:35Z

sdk/python/setup.py

What is this for?

This is a missing dependency because we are using the TensorFlow metadata proto

Previously I keep facing this bug whenever I tried to generate my proto. We verified on a clean install on @davidheryanto environment as well.

In fact @davidheryanto is the one who found it

Ok, but how is it related to your PR? Should it be a separate PR?

I think you're right, should created a separate PR
#508

This is the PR @woop, this also generates the python SDK Protos

This will help to reduce the file change in this PR, so we can focus on the features

feast-ci-bot · 2020-03-03T18:26:12Z

The following users are mentioned in OWNERS file(s) but are not members of the gojek org.

Once all users have been added as members of the org, you can trigger verification by writing /verify-owners in a comment.

imjuanleonard
- OWNERS

zhilingc · 2020-03-10T13:07:50Z

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

in TypeUtils we have helper functions convertJsonStringToMap and convertMapToJsonString, you might want to use those for converting to and from json.

imjuanleonard · 2020-03-11T04:08:49Z

/retest test-core-and-ingestion

zhilingc · 2020-03-11T04:28:55Z

/retest

imjuanleonard · 2020-03-17T21:23:11Z

All the comments in this merge have been addressed, it is ported to #536

imjuanleonard requested review from davidheryanto, khorshuheng, pradithya, woop and zhilingc as code owners February 27, 2020 03:24

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

feast-ci-bot added needs-ok-to-test size/M labels Feb 27, 2020

woop reviewed Feb 27, 2020

View reviewed changes

imjuanleonard force-pushed the feature-set-label-and-sdk branch from b460911 to 51b3111 Compare February 27, 2020 21:00

imjuanleonard closed this Feb 27, 2020

imjuanleonard force-pushed the feature-set-label-and-sdk branch from 51b3111 to d785b60 Compare February 27, 2020 21:00

feast-ci-bot added approved size/XS and removed size/M labels Feb 27, 2020

imjuanleonard reopened this Mar 2, 2020

feast-ci-bot removed the approved label Mar 2, 2020

feast-ci-bot added size/XXL and removed size/XS labels Mar 2, 2020

feast-ci-bot added ok-to-test do-not-merge/invalid-owners-file and removed needs-ok-to-test labels Mar 2, 2020

imjuanleonard force-pushed the feature-set-label-and-sdk branch 2 times, most recently from 2af7f5d to 9baa029 Compare March 2, 2020 09:13

woop reviewed Mar 2, 2020

View reviewed changes

imjuanleonard force-pushed the feature-set-label-and-sdk branch from 15ebc95 to 3f596a9 Compare March 3, 2020 18:25

zhilingc reviewed Mar 10, 2020

View reviewed changes

imjuanleonard force-pushed the feature-set-label-and-sdk branch 2 times, most recently from cd47be2 to abcce8c Compare March 10, 2020 23:21

feast-ci-bot added size/XS and removed size/XXL labels Mar 10, 2020

imjuanleonard force-pushed the feature-set-label-and-sdk branch from abcce8c to 8b1380f Compare March 10, 2020 23:44

Add labels with type text

f15e976

imjuanleonard force-pushed the feature-set-label-and-sdk branch from 8b1380f to f15e976 Compare March 10, 2020 23:45

imjuanleonard added 2 commits March 11, 2020 06:57

Add labels proto to feature spec

925bd43

Implement get labels from list feature set

c2764f6

feast-ci-bot added size/S and removed size/XS labels Mar 11, 2020

imjuanleonard added 2 commits March 11, 2020 07:52

Fix typo on CoreServiceImpl

eddc319

Add labels to Feature field proto generation

a1a7da7

imjuanleonard changed the title ~~[WIP] Do not merge - Feature set label and sdk access to it~~ Labels for feature Mar 11, 2020

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

imjuanleonard mentioned this pull request Mar 16, 2020

Add feature and feature set labels, for metadata #536

Merged

imjuanleonard closed this Mar 17, 2020

imjuanleonard deleted the feature-set-label-and-sdk branch March 17, 2020 21:23

Conversation

imjuanleonard commented Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feast-ci-bot commented Feb 27, 2020

Uh oh!

woop Feb 27, 2020

Choose a reason for hiding this comment

Uh oh!

imjuanleonard Feb 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woop Feb 28, 2020

Choose a reason for hiding this comment

Uh oh!

imjuanleonard Mar 4, 2020

Choose a reason for hiding this comment

Uh oh!

feast-ci-bot commented Mar 2, 2020

Uh oh!

feast-ci-bot commented Mar 2, 2020

Uh oh!

woop commented Mar 2, 2020

Uh oh!

woop Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

imjuanleonard Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woop Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

imjuanleonard Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

feast-ci-bot commented Mar 3, 2020

Uh oh!

zhilingc Mar 10, 2020

Choose a reason for hiding this comment

Uh oh!

imjuanleonard commented Mar 11, 2020

Uh oh!

zhilingc commented Mar 11, 2020

Uh oh!

imjuanleonard commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imjuanleonard commented Feb 27, 2020 •

edited

Loading

imjuanleonard Feb 27, 2020 •

edited

Loading

imjuanleonard Mar 2, 2020 •

edited

Loading

imjuanleonard commented Mar 17, 2020 •

edited

Loading