Create empty Job Service - #1059
Conversation
|
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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
|
/kind housekeeping |
048c380 to
92ef175
Compare
There was a problem hiding this comment.
we should make this a struct for consistency with the ingestion job parameter format
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
I think we should standardize the naming to created_timestamp, stop_timestamp since those are use eg in FeatureTable proto as well.
There was a problem hiding this comment.
Maybe standardize to project here?
There was a problem hiding this comment.
I recommend we follow pep8 style guide as stated here, to utilize lowercase separated by underscores for function names.
There was a problem hiding this comment.
JobService_pb2_grpc module name is autogenerated by grpc, I don't think we have a control to modify it.
ec84828 to
92ef175
Compare
Signed-off-by: Tsotne Tabidze <tsotnet@gmail.com>
92ef175 to
97f70be
Compare
Signed-off-by: Tsotne Tabidze <tsotnet@gmail.com>
bde318a to
653160e
Compare
| RUN make compile-protos-python | ||
|
|
||
| # Install Feast SDK | ||
| COPY .git .git |
There was a problem hiding this comment.
Is there a reason why we copy git into the docker file?
There was a problem hiding this comment.
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.
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
What this PR does / why we need it: This PR creates an empty Job Service with and the Docker image creation step.