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

Make Projects optional & Update Feature References #693

Merged
merged 21 commits into from
May 19, 2020
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6a61582
Squash and rebase on master
May 18, 2020
4b694d5
Change SpecServiceTest to use global default project constant.
May 18, 2020
68b9482
Make AccessManagementService's archiveProject() throw UnimplementOper…
May 18, 2020
1d74ce5
Correct incorrect documentation on source being ignored on Core's App…
May 18, 2020
179819a
Use ValueProto.Value in java SDK's FeastClient lambda instead of gene…
May 18, 2020
78c5fdd
Clarify how the ignore project param of feature ref to string() worke…
May 18, 2020
e9befcf
Correct missing test to check for empty feature refs in java SDK.
May 18, 2020
dce338d
Clarified ref_protos to feature_ref_protos to make code clearer
May 18, 2020
88cae23
Rename ServingService's FeatureReference proto 'feature_set_name` to …
May 18, 2020
e67abab
Remove nested function strip_project() with loop with python SDK's cl…
May 18, 2020
96b5fed
Remove extra version paramter in python sdk test's TestFeatureRef
May 18, 2020
11d1c31
Change CachedSpecService's "hasProject == true" to more concise "!has…
May 18, 2020
0aaf56e
Removed setFeatureSet() calls to OnlineServingServiceTest as it does …
May 18, 2020
ab9cdc6
Make client.set_project() without args reset project to default in py…
May 18, 2020
6bfcccb
Updated e2e to test for feature set inference for feature ref without…
May 18, 2020
12c7087
Remove unused field max_age in FeatureReference
May 18, 2020
19d1f8a
Fix typo in e2e
May 18, 2020
713b5bd
Fix error in e2e tests regarding string splits
May 18, 2020
15ac15e
Optimise Serving's CachedSpecService populateCache() by directly assi…
May 18, 2020
dbfec8d
Remove final in CachedSpecService to allow assignment in featureToFea…
May 18, 2020
adeb743
Remove unnescessary copy of list in QueryTemplater
May 18, 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
11 changes: 11 additions & 0 deletions core/src/main/java/feast/core/grpc/CoreServiceImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,17 @@ public void archiveProject(
accessManagementService.archiveProject(request.getName());
responseObserver.onNext(ArchiveProjectResponse.getDefaultInstance());
responseObserver.onCompleted();
} catch (IllegalArgumentException e) {
log.error("Recieved an invalid request on calling archiveProject method:", e);
responseObserver.onError(
Status.INVALID_ARGUMENT
.withDescription(e.getMessage())
.withCause(e)
.asRuntimeException());
} catch (UnsupportedOperationException e) {
log.error("Attempted to archive an unsupported project:", e);
responseObserver.onError(
Status.UNIMPLEMENTED.withDescription(e.getMessage()).withCause(e).asRuntimeException());
} catch (Exception e) {
log.error("Exception has occurred in the createProject method: ", e);
responseObserver.onError(
Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/feast/core/model/FeatureSet.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
uniqueConstraints = @UniqueConstraint(columnNames = {"name", "project_name"}))
public class FeatureSet extends AbstractTimestampEntity {

// Id of the featureSet, defined as project/feature_set_name:feature_set_version
@Id @GeneratedValue private long id;

// Name of the featureSet
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/feast/core/model/Project.java
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
@Entity
@Table(name = "projects")
public class Project {
public static final String DEFAULT_NAME = "default";
zhilingc marked this conversation as resolved.
Show resolved Hide resolved

// Name of the project
@Id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,15 @@
@Slf4j
@Service
public class AccessManagementService {

private ProjectRepository projectRepository;

@Autowired
public AccessManagementService(ProjectRepository projectRepository) {
this.projectRepository = projectRepository;
// create default project if it does not yet exist.
if (!projectRepository.existsById(Project.DEFAULT_NAME)) {
this.createProject(Project.DEFAULT_NAME);
}
}

/**
Expand Down Expand Up @@ -61,6 +64,9 @@ public void archiveProject(String name) {
if (!project.isPresent()) {
throw new IllegalArgumentException(String.format("Could not find project: \"%s\"", name));
}
if (name.equals(Project.DEFAULT_NAME)) {
throw new UnsupportedOperationException("Archiving the default project is not allowed.");
}
Project p = project.get();
p.setArchived(true);
projectRepository.saveAndFlush(p);
Expand Down
29 changes: 23 additions & 6 deletions core/src/main/java/feast/core/service/SpecService.java
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ public SpecService(
/**
* Get a feature set matching the feature name and version and project. The feature set name and
* project are required, but version can be omitted by providing 0 for its value. If the version
* is omitted, the latest feature set will be provided.
* is omitted, the latest feature set will be provided. If the project is omitted, the default
* would be used.
*
* @param request: GetFeatureSetRequest Request containing filter parameters.
* @return Returns a GetFeatureSetResponse containing a feature set..
Expand All @@ -94,8 +95,9 @@ public GetFeatureSetResponse getFeatureSet(GetFeatureSetRequest request)
if (request.getName().isEmpty()) {
throw new IllegalArgumentException("No feature set name provided");
}
// Autofill default project if project is not specified
if (request.getProject().isEmpty()) {
throw new IllegalArgumentException("No project provided");
request = request.toBuilder().setProject(Project.DEFAULT_NAME).build();
}

FeatureSet featureSet;
Expand All @@ -117,7 +119,8 @@ public GetFeatureSetResponse getFeatureSet(GetFeatureSetRequest request)
* projects.
*
* <p>Project name can be explicitly provided, or an asterisk can be provided to match all
* projects. It is not possible to provide a combination of asterisks/wildcards and text.
* projects. It is not possible to provide a combination of asterisks/wildcards and text. If the
* project name is omitted, the default project would be used.
*
* <p>The feature set name in the filter accepts an asterisk as a wildcard. All matching feature
* sets will be returned. Regex is not supported. Explicitly defining a feature set name is not
Expand All @@ -131,14 +134,19 @@ public ListFeatureSetsResponse listFeatureSets(ListFeatureSetsRequest.Filter fil
String name = filter.getFeatureSetName();
String project = filter.getProject();

if (project.isEmpty() || name.isEmpty()) {
if (name.isEmpty()) {
throw new IllegalArgumentException(
"Invalid listFeatureSetRequest, missing arguments. Must provide project and feature set name.");
"Invalid listFeatureSetRequest, missing arguments. Must provide feature set name:");
}

checkValidCharactersAllowAsterisk(name, "featureSetName");
checkValidCharactersAllowAsterisk(project, "projectName");

// Autofill default project if project not specified
if (project.isEmpty()) {
project = Project.DEFAULT_NAME;
}

List<FeatureSet> featureSets = new ArrayList<FeatureSet>() {};

if (project.contains("*")) {
Expand Down Expand Up @@ -227,12 +235,21 @@ public ListStoresResponse listStores(ListStoresRequest.Filter filter) {
*
* <p>This function is idempotent. If no changes are detected in the incoming featureSet's schema,
* this method will update the incoming featureSet spec with the latest version stored in the
* repository, and return that.
* repository, and return that. If project is not specified in the given featureSet, will assign
* the featureSet to the'default' project.
*
* @param newFeatureSet Feature set that will be created or updated.
*/
public ApplyFeatureSetResponse applyFeatureSet(FeatureSetProto.FeatureSet newFeatureSet)
throws InvalidProtocolBufferException {
// Autofill default project if not specified
if (newFeatureSet.getSpec().getProject().isEmpty()) {
newFeatureSet =
newFeatureSet
.toBuilder()
.setSpec(newFeatureSet.getSpec().toBuilder().setProject(Project.DEFAULT_NAME).build())
.build();
}

// Validate incoming feature set
FeatureSetValidator.validateSpec(newFeatureSet);
Expand Down
3 changes: 1 addition & 2 deletions core/src/test/java/feast/core/job/JobUpdateTaskTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ public class JobUpdateTaskTest {

private static final FeatureSetProto.FeatureSet.Builder fsBuilder =
FeatureSetProto.FeatureSet.newBuilder().setMeta(FeatureSetMeta.newBuilder());
private static final FeatureSetSpec.Builder specBuilder =
FeatureSetSpec.newBuilder().setProject("project1");
private static final FeatureSetSpec.Builder specBuilder = FeatureSetSpec.newBuilder();

@Mock private JobManager jobManager;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* SPDX-License-Identifier: Apache-2.0
* Copyright 2018-2019 The Feast Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package feast.core.service;

import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.mockito.MockitoAnnotations.initMocks;

import feast.core.dao.ProjectRepository;
import feast.core.model.Project;
import java.util.Optional;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExpectedException;
import org.mockito.Mock;

public class AccessManagementServiceTest {
@Rule public ExpectedException expectedException = ExpectedException.none();
// mocks
@Mock private ProjectRepository projectRepository;
// dummy models
private Project defaultProject;
private Project testProject;

// test target
private AccessManagementService accessService;

@Before
public void setup() {
initMocks(this);
// setup dummy models for testing
this.defaultProject = new Project(Project.DEFAULT_NAME);
this.testProject = new Project("project");
// setup test target
when(this.projectRepository.existsById(Project.DEFAULT_NAME)).thenReturn(false);
this.accessService = new AccessManagementService(this.projectRepository);
}

@Test
public void testDefaultProjectCreateInConstructor() {
verify(this.projectRepository).saveAndFlush(this.defaultProject);
}

@Test
public void testArchiveProject() {
when(this.projectRepository.findById("project")).thenReturn(Optional.of(this.testProject));
this.accessService.archiveProject("project");
this.testProject.setArchived(true);
verify(this.projectRepository).saveAndFlush(this.testProject);
// reset archived flag
this.testProject.setArchived(false);
}

@Test
public void shouldNotArchiveDefaultProject() {
expectedException.expect(IllegalArgumentException.class);
this.accessService.archiveProject(Project.DEFAULT_NAME);
}
}
8 changes: 0 additions & 8 deletions core/src/test/java/feast/core/service/JobServiceTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ public class JobServiceTest {
// test target
public JobService jobService;

/* unit test setup */
@Before
public void setup() {
initMocks(this);
Expand Down Expand Up @@ -107,7 +106,6 @@ public void setup() {
new JobService(this.jobRepository, this.specService, Arrays.asList(this.jobManager));
}

// setup fake spec service
public void setupSpecService() {
try {
ListFeatureSetsResponse response =
Expand All @@ -124,7 +122,6 @@ public void setupSpecService() {
}
}

// setup fake job repository
public void setupJobRepository() {
when(this.jobRepository.findById(this.job.getId())).thenReturn(Optional.of(this.job));
when(this.jobRepository.findByStoreName(this.dataStore.getName()))
Expand All @@ -134,14 +131,12 @@ public void setupJobRepository() {
when(this.jobRepository.findAll()).thenReturn(Arrays.asList(this.job));
}

// TODO: setup fake job manager
public void setupJobManager() {
when(this.jobManager.getRunnerType()).thenReturn(Runner.DATAFLOW);
when(this.jobManager.restartJob(this.job))
.thenReturn(this.newDummyJob(this.job.getId(), this.job.getExtId(), JobStatus.PENDING));
}

// dummy model constructorss
private FeatureSet newDummyFeatureSet(String name, int version, String project) {
Feature feature = TestObjectFactory.CreateFeature(name + "_feature", Enum.INT64);
Entity entity = TestObjectFactory.CreateEntity(name + "_entity", Enum.STRING);
Expand Down Expand Up @@ -203,7 +198,6 @@ private List<ListFeatureSetsRequest.Filter> newDummyListRequestFilters() {
.build());
}

/* unit tests */
private ListIngestionJobsResponse tryListJobs(ListIngestionJobsRequest request) {
ListIngestionJobsResponse response = null;
try {
Expand All @@ -216,7 +210,6 @@ private ListIngestionJobsResponse tryListJobs(ListIngestionJobsRequest request)
return response;
}

// list jobs
@Test
public void testListJobsById() {
ListIngestionJobsRequest.Filter filter =
Expand Down Expand Up @@ -275,7 +268,6 @@ public void testListIngestionJobByFeatureSetReference() {
assertThat(this.tryListJobs(request).getJobs(0), equalTo(this.ingestionJob));
}

// stop jobs
private StopIngestionJobResponse tryStopJob(
StopIngestionJobRequest request, boolean expectError) {
StopIngestionJobResponse response = null;
Expand Down
56 changes: 40 additions & 16 deletions core/src/test/java/feast/core/service/SpecServiceTest.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/*
* SPDX-License-Identifier: Apache-2.0
* Copyright 2018-2019 The Feast Authors
* Copyright 2018-2020 The Feast Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -34,6 +34,7 @@
import feast.proto.core.CoreServiceProto.ApplyFeatureSetResponse;
import feast.proto.core.CoreServiceProto.ApplyFeatureSetResponse.Status;
import feast.proto.core.CoreServiceProto.GetFeatureSetRequest;
import feast.proto.core.CoreServiceProto.GetFeatureSetResponse;
import feast.proto.core.CoreServiceProto.ListFeatureSetsRequest.Filter;
import feast.proto.core.CoreServiceProto.ListFeatureSetsResponse;
import feast.proto.core.CoreServiceProto.ListStoresRequest;
Expand Down Expand Up @@ -105,11 +106,13 @@ public void setUp() {
Feature f3f1 = TestObjectFactory.CreateFeature("f3f1", Enum.INT64);
Feature f3f2 = TestObjectFactory.CreateFeature("f3f2", Enum.INT64);
Entity f3e1 = TestObjectFactory.CreateEntity("f3e1", Enum.STRING);
FeatureSet featureSet3v1 =
FeatureSet featureSet3 =
TestObjectFactory.CreateFeatureSet(
"f3", "project1", Arrays.asList(f3e1), Arrays.asList(f3f2, f3f1));

featureSets = Arrays.asList(featureSet1, featureSet2);
FeatureSet featureSet4 = newDummyFeatureSet("f4", Project.DEFAULT_NAME);
featureSets = Arrays.asList(featureSet1, featureSet2, featureSet3, featureSet4);

when(featureSetRepository.findAll()).thenReturn(featureSets);
when(featureSetRepository.findAllByOrderByNameAsc()).thenReturn(featureSets);
when(featureSetRepository.findFeatureSetByNameAndProject_Name("f1", "project1"))
Expand Down Expand Up @@ -160,15 +163,6 @@ public void shouldGetAllFeatureSetsIfOnlyWildcardsProvided()
assertThat(actual, equalTo(expected));
}

@Test
public void listFeatureSetShouldFailIfFeatureSetProvidedWithoutProject()
throws InvalidProtocolBufferException {
expectedException.expect(IllegalArgumentException.class);
expectedException.expectMessage(
"Invalid listFeatureSetRequest, missing arguments. Must provide project and feature set name.");
specService.listFeatureSets(Filter.newBuilder().setFeatureSetName("f1").build());
}

@Test
public void shouldGetAllFeatureSetsMatchingNameWithWildcardSearch()
throws InvalidProtocolBufferException {
Expand Down Expand Up @@ -511,6 +505,26 @@ public void applyFeatureSetShouldCreateProjectWhenNotAlreadyExists()
equalTo(incomingFeatureSet.getSpec().getProject()));
}

@Test
public void applyFeatureSetShouldUsedDefaultProjectIfUnspecified()
throws InvalidProtocolBufferException {
Feature f3f1 = TestObjectFactory.CreateFeature("f3f1", Enum.INT64);
Feature f3f2 = TestObjectFactory.CreateFeature("f3f2", Enum.INT64);
Entity f3e1 = TestObjectFactory.CreateEntity("f3e1", Enum.STRING);

// In protov3, unspecified project defaults to ""
FeatureSetProto.FeatureSet incomingFeatureSet =
TestObjectFactory.CreateFeatureSet("f3", "", Arrays.asList(f3e1), Arrays.asList(f3f2, f3f1))
.toProto();
ApplyFeatureSetResponse applyFeatureSetResponse =
specService.applyFeatureSet(incomingFeatureSet);
assertThat(applyFeatureSetResponse.getStatus(), equalTo(Status.CREATED));

assertThat(
applyFeatureSetResponse.getFeatureSet().getSpec().getProject(),
equalTo(Project.DEFAULT_NAME));
}

@Test
public void applyFeatureSetShouldFailWhenProjectIsArchived()
throws InvalidProtocolBufferException {
Expand Down Expand Up @@ -661,10 +675,20 @@ public void shouldDoNothingIfNoChange() throws InvalidProtocolBufferException {
}

@Test
public void shouldFailIfGetFeatureSetWithoutProject() throws InvalidProtocolBufferException {
expectedException.expect(IllegalArgumentException.class);
expectedException.expectMessage("No project provided");
specService.getFeatureSet(GetFeatureSetRequest.newBuilder().setName("f1").build());
public void getOrListFeatureSetShouldUseDefaultProjectIfProjectUnspecified()
throws InvalidProtocolBufferException {
when(featureSetRepository.findFeatureSetByNameAndProject_Name("f4", Project.DEFAULT_NAME))
woop marked this conversation as resolved.
Show resolved Hide resolved
.thenReturn(featureSets.get(3));
FeatureSet expected = featureSets.get(3);
// check getFeatureSet()
GetFeatureSetResponse getResponse =
specService.getFeatureSet(GetFeatureSetRequest.newBuilder().setName("f4").build());
assertThat(getResponse.getFeatureSet(), equalTo(expected.toProto()));

// check listFeatureSets()
ListFeatureSetsResponse listResponse =
specService.listFeatureSets(Filter.newBuilder().setFeatureSetName("f4").build());
assertThat(listResponse.getFeatureSetsList(), equalTo(Arrays.asList(expected.toProto())));
}

private FeatureSet newDummyFeatureSet(String name, String project) {
Expand Down
Loading