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.

Sign up for GitHub

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

Jump to bottom

Implement JobService API calls & connect it to SDK #1111

Closed
wants to merge 12 commits into from
Closed

Implement JobService API calls & connect it to SDK #1111

wants to merge 12 commits into from

Conversation

tsotnet
Copy link
Collaborator

@tsotnet tsotnet commented Oct 29, 2020

What this PR does / why we need it:

  • Implement start_offline_to_online_ingestion, start_historical_feature_retrieval_job, start_stream_to_online_ingestion SDK calls with JobService (including branching logic depending on whether job service is enabled or not)
  • Set correct environment for JobService (Helm Charts & Dockerfile)
  • Update JobService protos as necessary (according to Slack discussion)

@tsotnet tsotnet requested a review from oavdeev October 29, 2020 05:56
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

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

@tsotnet tsotnet added the kind/feature New feature or request label Oct 29, 2020
woop
woop requested changes Oct 29, 2020
View reviewed changes
sdk/python/feast/constants.py Outdated Show resolved Hide resolved
oavdeev
oavdeev reviewed Oct 29, 2020
View reviewed changes
infra/docker/jobservice/Dockerfile Show resolved Hide resolved
infra/docker/jobservice/Dockerfile Outdated Show resolved Hide resolved
@tsotnet
Python lint
2c055f7 
Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Merge branch 'master' into job-service-server
0296462 
Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Correct implementation of all jobservice functions tested on standalo…
7e68055 
…ne mode

* New API calls (start_offline_to_online_ingestion, start_stream_to_online_ingestion) now return Remote Jobs instead of job ids
* Implement list_jobs & get_job for standalone mode (looks like Spark is running in local mode and we can't get job statuses so we have to keep cache in memory)
* Wire up list_jobs & get_job on client side with job service
* Tested locally on Feast 101 notebook, everything works

Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Fix list_jobs when include_terminated=False
f59dc02 
Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Copy link
Collaborator Author

tsotnet commented Oct 30, 2020

/test python-sdk-integration-test

@tsotnet
e2e tests
e5506e7 
Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Merge branch 'master' into job-service-server
84f6e4c 
Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Format python
afe6e00 
Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet tsotnet changed the title Implement half of JobService functionality Implement JobService API calls & connect it to SDK Nov 2, 2020
@tsotnet
Copy link
Collaborator Author

tsotnet commented Nov 3, 2020

/test test-end-to-end

@tsotnet
Copy link
Collaborator Author

tsotnet commented Nov 3, 2020

/test test-end-to-end-aws

pyalex
pyalex reviewed Nov 4, 2020
View reviewed changes
sdk/python/feast/job_service.py
feature_table = self.client.get_feature_table(
request.table_name, request.project
)
job = self.client.start_offline_to_online_ingestion(
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use launcher.start_offline_to_online instead of client here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

the reason is: client may have different (user-oriented) API (like accepting strings instead of feature table, dataframes instead of source) and it's not really idempotent (and being already called once)

Copy link
Collaborator Author

@tsotnet tsotnet Nov 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Done in the fork: #1129. Please comment there if anything else is needed.

@tsotnet
Copy link
Collaborator Author

tsotnet commented Nov 4, 2020

Closing this and instead opening a PR in a fork: #1129

@tsotnet tsotnet closed this Nov 4, 2020
@tsotnet tsotnet deleted the job-service-server branch November 4, 2020 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woop woop woop requested changes

@pyalex pyalex pyalex left review comments

@oavdeev oavdeev oavdeev left review comments

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@khorshuheng khorshuheng Awaiting requested review from khorshuheng

@zhilingc zhilingc Awaiting requested review from zhilingc

Assignees
No one assigned
Labels
kind/feature New feature or request size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tsotnet @feast-ci-bot @oavdeev @pyalex @woop