Create empty Job Service #1059

tsotnet · 2020-10-15T13:15:40Z

What this PR does / why we need it: This PR creates an empty Job Service with and the Docker image creation step.

feast-ci-bot · 2020-10-15T13:15:58Z

Hi @tsotnet. Thanks for your PR.

I'm waiting for a feast-dev 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.

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-10-15T13:43:22Z

/ok-to-test

woop · 2020-10-15T13:43:30Z

/kind housekeeping

woop · 2020-10-16T09:54:55Z

infra/docker/jobservice/Dockerfile

+COPY sdk/python sdk/python
+COPY Makefile Makefile
+COPY protos protos
+COPY tests tests


Do we need the tests?

woop · 2020-10-16T09:55:53Z

protos/feast/core/JobService.proto

+    // Produce a training dataset, return a job id that will provide a file reference
+    rpc GetHistoricalFeatures (GetHistoricalFeaturesRequest) returns (GetHistoricalFeaturesResponse);
+
+    //   IMPORTANT: The following RPCs are not must-haves for the MVP. They are stretch goals. All of these methods would exist internally in the job service and would be called by a control loop to start/stop/restart streaming ingestion jobs.


We can probably just remove this.

oavdeev · 2020-10-16T10:08:35Z

protos/feast/core/JobService.proto

+    string project = 1;
+    string table_name = 2;
+    // Connection String to the Redis Online Store that the job is writing to.
+    string redis_connection_string = 3;


we should make this a struct for consistency with the ingestion job parameter format

Actually lets just drop it from here for now. The only way this entire Job proto is used is to list jobs, i think it is fine if you just get the basic info like job type and table name there, but not redis connection details.

We can always add more stuff later.

terryyylim

I think we should standardize the naming to created_timestamp, stop_timestamp since those are use eg in FeatureTable proto as well.

terryyylim · 2020-10-17T00:25:48Z

protos/feast/core/JobService.proto

+  // Optional field to specify project name override. If specified, uses the
+  // given project for retrieval. Overrides the projects specified in
+  // Feature References if both are specified.
+  string project_name = 3;


Maybe standardize to project here?

terryyylim · 2020-10-17T00:30:58Z

sdk/python/feast/job_service.py

+from feast.core import JobService_pb2_grpc
+
+
+class JobServiceServicer(JobService_pb2_grpc.JobServiceServicer):


I recommend we follow pep8 style guide as stated here, to utilize lowercase separated by underscores for function names.

JobService_pb2_grpc module name is autogenerated by grpc, I don't think we have a control to modify it.

infra/charts/feast/charts/feast-jobservice/templates/deployment.yaml

Signed-off-by: Tsotne Tabidze <tsotnet@gmail.com>

khorshuheng · 2020-10-21T14:51:55Z

infra/docker/jobservice/Dockerfile

+RUN make compile-protos-python
+
+# Install Feast SDK
+COPY .git .git


Is there a reason why we copy git into the docker file?

Looks like git is used by sdk/python/setup.py here https://github.com/feast-dev/feast/blob/master/sdk/python/setup.py#L52. If I remove this line docker build step errors out.

Also I copied this mostly from the jupyter dockerfile, which contains the similar build step. We could improve docker build files by removing the dependency on git (seems like it's only used to copy the readme file from the git repo root), but I think that's probably better handled by another PR.

feast-ci-bot · 2020-10-21T15:02:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tsotnet, 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-10-21T15:02:56Z

/lgtm

tsotnet requested review from davidheryanto, khorshuheng, pyalex, woop and zhilingc as code owners October 15, 2020 13:15

feast-ci-bot added needs-kind needs-ok-to-test size/L labels Oct 15, 2020

feast-ci-bot added ok-to-test and removed needs-ok-to-test labels Oct 15, 2020

feast-ci-bot added kind/housekeeping and removed needs-kind labels Oct 15, 2020

tsotnet force-pushed the job-service-creation branch from 048c380 to 92ef175 Compare October 15, 2020 13:55

woop reviewed Oct 16, 2020

View reviewed changes

oavdeev reviewed Oct 16, 2020

View reviewed changes

terryyylim reviewed Oct 17, 2020

View reviewed changes

feast-ci-bot added size/XL and removed size/L labels Oct 21, 2020

oavdeev reviewed Oct 21, 2020

View reviewed changes

infra/charts/feast/charts/feast-jobservice/templates/deployment.yaml Outdated Show resolved Hide resolved

tsotnet force-pushed the job-service-creation branch from ec84828 to 92ef175 Compare October 21, 2020 09:13

feast-ci-bot added size/L and removed size/XL labels Oct 21, 2020

Create empty Job Service

97f70be

Signed-off-by: Tsotne Tabidze <tsotnet@gmail.com>

tsotnet force-pushed the job-service-creation branch from 92ef175 to 97f70be Compare October 21, 2020 14:25

Reintroduce accidentally removed functions in JobServiceServicer

653160e

Signed-off-by: Tsotne Tabidze <tsotnet@gmail.com>

tsotnet force-pushed the job-service-creation branch from bde318a to 653160e Compare October 21, 2020 14:35

khorshuheng reviewed Oct 21, 2020

View reviewed changes

woop approved these changes Oct 21, 2020

View reviewed changes

feast-ci-bot added the approved label Oct 21, 2020

feast-ci-bot assigned woop Oct 21, 2020

feast-ci-bot added the lgtm label Oct 21, 2020

feast-ci-bot merged commit 473e0af into feast-dev:master Oct 21, 2020

tsotnet deleted the job-service-creation branch October 21, 2020 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create empty Job Service #1059

Create empty Job Service #1059

tsotnet commented Oct 15, 2020

feast-ci-bot commented Oct 15, 2020

woop commented Oct 15, 2020

woop commented Oct 15, 2020

woop Oct 16, 2020

woop Oct 16, 2020

oavdeev Oct 16, 2020

oavdeev-tt Oct 21, 2020

terryyylim left a comment

terryyylim Oct 17, 2020

terryyylim Oct 17, 2020

tsotnet Oct 21, 2020 •

edited

Loading

khorshuheng Oct 21, 2020

tsotnet Oct 21, 2020

feast-ci-bot commented Oct 21, 2020

woop commented Oct 21, 2020

		from feast.core import JobService_pb2_grpc


		class JobServiceServicer(JobService_pb2_grpc.JobServiceServicer):

Create empty Job Service #1059

Create empty Job Service #1059

Conversation

tsotnet commented Oct 15, 2020

feast-ci-bot commented Oct 15, 2020

woop commented Oct 15, 2020

woop commented Oct 15, 2020

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

terryyylim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsotnet Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feast-ci-bot commented Oct 21, 2020

woop commented Oct 21, 2020

tsotnet Oct 21, 2020 •

edited

Loading