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

Add support to Python SDK for staging files on Amazon S3 #769

Merged
merged 10 commits into from
Jun 28, 2020
Merged

Add support to Python SDK for staging files on Amazon S3 #769

merged 10 commits into from
Jun 28, 2020

Conversation

jmelinav
Copy link
Collaborator

@jmelinav jmelinav commented Jun 4, 2020 •
edited by woop
Loading

What this PR does / why we need it:
Currently Feast client cannot stage data anywhere except GCP storage. The PR adds S3 staging support on client.
Which issue(s) this PR fixes: Client can stage files on s3.

Python Client changes for #706.
Edit: Fixes #562

Does this PR introduce a user-facing change?: No

Added support to Python SDK for staging files on Amazon S3

@feast-ci-bot
Copy link
Collaborator

Hi @jmelinav. 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.

@dr3s dr3s force-pushed the feature-s3-staging branch 4 times, most recently from eded390 to c1e1d91 Compare June 5, 2020 00:00
@dr3s
Copy link
Collaborator

dr3s commented Jun 5, 2020

/ok-to-test

@dr3s dr3s mentioned this pull request Jun 5, 2020
Closed
@jmelinav
Copy link
Collaborator Author

jmelinav commented Jun 5, 2020

/test test-end-to-end

@jmelinav
Copy link
Collaborator Author

jmelinav commented Jun 5, 2020

/assign @pyalex

@dr3s dr3s added the kind/feature New feature or request label Jun 5, 2020
@woop
Copy link
Member

woop commented Jun 7, 2020

Can you please also add a note in the release log that you have added S3 support?

woop
woop reviewed Jun 7, 2020
View reviewed changes
sdk/python/feast/job.py Outdated Show resolved Hide resolved
@jmelinav jmelinav changed the title Add support for staging files on S3 Add support for staging files on S3 - Client side only (python sdk) Jun 25, 2020
@jmelinav
Copy link
Collaborator Author

jmelinav commented Jun 25, 2020 •
edited
Loading

Since this PR only addresses the support on the Python SDK client side, should we:

  • the retitle the PR as such lest people start thinking that full S3 staging support is addressed in the PR
  • not close the S3 staging issue with this PR until Serving side has something that supports S3 staging.

Sounds good to me, I have removed the fixes #706 from the PR description and mentioned it addresses only python client changes. Also I have appended to Client side only(python sdk). Feel free to retitle.

@woop
Copy link
Member

woop commented Jun 28, 2020

/test test-end-to-end-batch

@jmelinav
Copy link
Collaborator Author

/test test-end-to-end

@jmelinav
Copy link
Collaborator Author

/test test-end-to-end-auth

@jmelinav
Copy link
Collaborator Author

/test test-end-to-end-batch

@jmelinav
Copy link
Collaborator Author

/test test-end-to-end

@woop woop changed the title Add support for staging files on S3 - Client side only (python sdk) Add support to Python SDK to stage files on Amazon S3 Jun 28, 2020
@woop woop changed the title Add support to Python SDK to stage files on Amazon S3 Add support to Python SDK for staging files on Amazon S3 Jun 28, 2020
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmelinav, 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:

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

@woop
Copy link
Member

woop commented Jun 28, 2020

/lgtm

@feast-ci-bot feast-ci-bot merged commit 4da4b23 into feast-dev:master Jun 28, 2020
@algattik algattik mentioned this pull request Jun 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrzzy mrzzy mrzzy left review comments

@woop woop woop approved these changes

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@khorshuheng khorshuheng Awaiting requested review from khorshuheng

@pyalex pyalex Awaiting requested review from pyalex

@zhilingc zhilingc Awaiting requested review from zhilingc

Assignees

@pyalex pyalex

@woop woop

Labels
approved kind/feature New feature or request lgtm ok-to-test size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove GCP dependency in test_get_batch_features
8 participants
@jmelinav @feast-ci-bot @dr3s @woop @algattik @mrzzy @pyalex @terryyylim