Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature and feature set labels, for metadata #536

Merged
merged 25 commits into from
Apr 30, 2020
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e2d731c
Add labels column to feature field
imjuanleonard Mar 22, 2020
0cdb63b
Add labels to feature spec proto
imjuanleonard Mar 22, 2020
6802e0a
Implement labels on feature model
imjuanleonard Mar 22, 2020
60e4cc8
Implement list labels
imjuanleonard Mar 22, 2020
b597799
Implement set_label and remove_label from feature client
imjuanleonard Mar 22, 2020
4e69b2e
Refactor ingest to only accept featureset and dtype
imjuanleonard Mar 22, 2020
258560c
Add equality on labels field
imjuanleonard Mar 24, 2020
ced6609
Add tests for labels apply
imjuanleonard Mar 24, 2020
1a06833
Add Optional type hint to labels
imjuanleonard Mar 30, 2020
9e768a1
Add empty labels key validation
imjuanleonard Mar 30, 2020
108de14
Add label when generating feature spec for specServiceTest to test eq…
imjuanleonard Apr 1, 2020
c6cf42c
corrected convention (push test)
Apr 24, 2020
b9fd9af
corrected review comments
Apr 26, 2020
f241a4f
Merge branch 'master' into python-sdk-for-labels
Apr 27, 2020
dd88b6c
corrected lint-python check
Apr 27, 2020
2f7c86a
corrected lint-python 2
Apr 27, 2020
30d63fa
back out python SDK changes
Apr 28, 2020
4b7ec66
Implemented labels on a feature set level
Apr 28, 2020
b3a6317
added empty keys validation
Apr 28, 2020
de1153e
corrected review comments (storing empty json for features if labels …
Apr 29, 2020
a756929
Updated the comment to match the logic
suwik Apr 29, 2020
18edc12
added e2e tests for feature and feature set labels
Apr 30, 2020
5e10c94
Merge branch 'python-sdk-for-labels' of https://github.com/imjuanleon…
Apr 30, 2020
02197f8
moved e2e tests for feature and feature set labels
Apr 30, 2020
8d6d4f4
changed tests ordering
Apr 30, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions core/src/main/java/feast/core/model/FeatureSet.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import feast.core.FeatureSetProto.FeatureSetSpec;
import feast.core.FeatureSetProto.FeatureSetStatus;
import feast.core.FeatureSetProto.FeatureSpec;
import feast.core.util.TypeConversion;
import feast.types.ValueProto.ValueType.Enum;
import java.util.ArrayList;
import java.util.HashMap;
Expand Down Expand Up @@ -116,6 +117,10 @@ public class FeatureSet extends AbstractTimestampEntity implements Comparable<Fe
@Column(name = "status")
private String status;

// User defined metadata
@Column(name = "labels", columnDefinition = "text")
private String labels;

public FeatureSet() {
super();
}
Expand All @@ -128,6 +133,7 @@ public FeatureSet(
List<Field> entities,
List<Field> features,
Source source,
Map<String, String> labels,
FeatureSetStatus status) {
this.maxAgeSeconds = maxAgeSeconds;
this.source = source;
Expand All @@ -137,6 +143,7 @@ public FeatureSet(
this.name = name;
this.project = new Project(project);
this.version = version;
this.labels = TypeConversion.convertMapToJsonString(labels);
this.setId(project, name, version);
addEntities(entities);
addFeatures(features);
Expand Down Expand Up @@ -191,6 +198,7 @@ public static FeatureSet fromProto(FeatureSetProto.FeatureSet featureSetProto) {
entitySpecs,
featureSpecs,
source,
featureSetProto.getSpec().getLabelsMap(),
featureSetProto.getMeta().getStatus());
}

Expand Down Expand Up @@ -247,6 +255,7 @@ public FeatureSetProto.FeatureSet toProto() throws InvalidProtocolBufferExceptio
.setMaxAge(Duration.newBuilder().setSeconds(maxAgeSeconds))
.addAllEntities(entitySpecs)
.addAllFeatures(featureSpecs)
.putAllLabels(TypeConversion.convertJsonStringToMap(labels))
.setSource(source.toProto());

return FeatureSetProto.FeatureSet.newBuilder().setMeta(meta).setSpec(spec).build();
Expand Down Expand Up @@ -352,6 +361,10 @@ private void setFeatureSpecFields(FeatureSpec.Builder featureSpecBuilder, Field
featureSpecBuilder.setTimeOfDayDomain(
TimeOfDayDomain.parseFrom(featureField.getTimeOfDayDomain()));
}

if (featureField.getLabels() != null) {
featureSpecBuilder.putAllLabels(featureField.getLabels());
}
}

/**
Expand Down
20 changes: 13 additions & 7 deletions core/src/main/java/feast/core/model/Field.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@

import feast.core.FeatureSetProto.EntitySpec;
import feast.core.FeatureSetProto.FeatureSpec;
import feast.types.ValueProto.ValueType;
import feast.core.util.TypeConversion;
import java.util.Arrays;
import java.util.Map;
import java.util.Objects;
import javax.persistence.Column;
import javax.persistence.Embeddable;
Expand Down Expand Up @@ -47,6 +48,10 @@ public class Field {
@Column(name = "project")
private String project;

// Labels that this field belongs to
@Column(name = "labels", columnDefinition = "text")
private String labels;
Copy link
Member

@ches ches Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm so I guess we're going to want to move this to the Feature entity class in #655… this was mentioned before but I guess I wasn't understanding the implications until I saw #655 reduced from #612.

Creates a bit of a conundrum for us since I thought the labels feature could be a straightforward backport (internal, assuming Feast isn't going to backport features). That's not going to be the case if this has to wait on #655 and substantial SQL schema migrations are going to be required for it.


// Presence constraints (refer to proto feast.core.FeatureSet.FeatureSpec)
// Only one of them can be set.
private byte[] presence;
Expand Down Expand Up @@ -74,14 +79,10 @@ public class Field {

public Field() {}

public Field(String name, ValueType.Enum type) {
this.name = name;
this.type = type.toString();
}

public Field(FeatureSpec featureSpec) {
this.name = featureSpec.getName();
this.type = featureSpec.getValueType().toString();
this.labels = TypeConversion.convertMapToJsonString(featureSpec.getLabelsMap());

switch (featureSpec.getPresenceConstraintsCase()) {
case PRESENCE:
Expand Down Expand Up @@ -215,6 +216,10 @@ public Field(EntitySpec entitySpec) {
}
}

public Map<String, String> getLabels() {
return TypeConversion.convertJsonStringToMap(this.labels);
}

@Override
public boolean equals(Object o) {
if (this == o) {
Expand All @@ -227,6 +232,7 @@ public boolean equals(Object o) {
return Objects.equals(name, field.name)
&& Objects.equals(type, field.type)
&& Objects.equals(project, field.project)
&& Objects.equals(labels, field.labels)
&& Arrays.equals(presence, field.presence)
&& Arrays.equals(groupPresence, field.groupPresence)
&& Arrays.equals(shape, field.shape)
Expand All @@ -247,6 +253,6 @@ public boolean equals(Object o) {

@Override
public int hashCode() {
return Objects.hash(super.hashCode(), name, type);
return Objects.hash(super.hashCode(), name, type, project, labels);
}
}
5 changes: 1 addition & 4 deletions core/src/main/java/feast/core/util/TypeConversion.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,12 +67,9 @@ public static Map<String, String> convertJsonStringToMap(String jsonString) {
* Marshals a given map into its corresponding json string
*
* @param map
* @return json string corresponding to given map
* @return json string corresponding to given map or null if the map is empty
*/
public static String convertMapToJsonString(Map<String, String> map) {
ches marked this conversation as resolved.
Show resolved Hide resolved
if (map.isEmpty()) {
return "{}";
}
return gson.toJson(map);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,17 @@
import java.util.stream.Collectors;

public class FeatureSetValidator {

public static void validateSpec(FeatureSet featureSet) {
if (featureSet.getSpec().getProject().isEmpty()) {
throw new IllegalArgumentException("Project name must be provided");
}
if (featureSet.getSpec().getName().isEmpty()) {
throw new IllegalArgumentException("Feature set name must be provided");
}
if (featureSet.getSpec().getLabelsMap().containsKey("")) {
throw new IllegalArgumentException("Feature set label keys must not be empty");
}

checkValidCharacters(featureSet.getSpec().getProject(), "project");
checkValidCharacters(featureSet.getSpec().getName(), "name");
Expand All @@ -44,6 +48,9 @@ public static void validateSpec(FeatureSet featureSet) {
}
for (FeatureSpec featureSpec : featureSet.getSpec().getFeaturesList()) {
checkValidCharacters(featureSpec.getName(), "features::name");
if (featureSpec.getLabelsMap().containsKey("")) {
throw new IllegalArgumentException("Feature label keys must not be empty");
}
}
}

Expand Down
27 changes: 5 additions & 22 deletions core/src/test/java/feast/core/service/JobServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,8 @@
import feast.core.CoreServiceProto.RestartIngestionJobResponse;
import feast.core.CoreServiceProto.StopIngestionJobRequest;
import feast.core.CoreServiceProto.StopIngestionJobResponse;
import feast.core.FeatureSetProto.FeatureSetStatus;
import feast.core.FeatureSetReferenceProto.FeatureSetReference;
import feast.core.IngestionJobProto.IngestionJob;
import feast.core.SourceProto.KafkaSourceConfig;
import feast.core.SourceProto.SourceType;
import feast.core.StoreProto.Store.RedisConfig;
import feast.core.StoreProto.Store.StoreType;
import feast.core.dao.JobRepository;
Expand Down Expand Up @@ -84,14 +81,7 @@ public void setup() {

// create mock objects for testing
// fake data source
this.dataSource =
new Source(
SourceType.KAFKA,
KafkaSourceConfig.newBuilder()
.setBootstrapServers("kafka:9092")
.setTopic("my-topic")
.build(),
true);
this.dataSource = TestObjectFactory.defaultSource;
// fake data store
this.dataStore =
new Store(
Expand Down Expand Up @@ -158,19 +148,12 @@ public void setupJobManager() {

// dummy model constructorss
private FeatureSet newDummyFeatureSet(String name, int version, String project) {
Field feature = new Field(name + "_feature", Enum.INT64);
Field entity = new Field(name + "_entity", Enum.STRING);
Field feature = TestObjectFactory.CreateFeatureField(name + "_feature", Enum.INT64);
Field entity = TestObjectFactory.CreateEntityField(name + "_entity", Enum.STRING);

FeatureSet fs =
new FeatureSet(
name,
project,
version,
100L,
Arrays.asList(entity),
Arrays.asList(feature),
this.dataSource,
FeatureSetStatus.STATUS_READY);
TestObjectFactory.CreateFeatureSet(
name, project, version, Arrays.asList(entity), Arrays.asList(feature));
fs.setCreated(Date.from(Instant.ofEpochSecond(10L)));
return fs;
}
Expand Down
Loading