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

S3 endpoint configuration #1169 #1172

Merged
merged 12 commits into from
Nov 22, 2020
Merged

S3 endpoint configuration #1169 #1172

merged 12 commits into from
Nov 22, 2020

Conversation

mike0sv
Copy link
Contributor

@mike0sv mike0sv commented Nov 16, 2020

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1169

Does this PR introduce a user-facing change?:

Added an option to configure S3 endpoint url

@feast-ci-bot
Copy link
Collaborator

Hi @mike0sv. 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
Copy link
Member

woop commented Nov 17, 2020

Thanks for this @mike0sv!

/ok-to-test

@woop
Copy link
Member

woop commented Nov 17, 2020

/kind feature

@feast-ci-bot feast-ci-bot added kind/feature New feature or request and removed needs-kind labels Nov 17, 2020
@mike0sv
Copy link
Contributor Author

mike0sv commented Nov 17, 2020

Hey, @woop, some tests failed because I provided None as default value. There are two possible solutions, 1) add allow_no_value=True to ConfigParser so it will tolerate Nones 2) Set default to https://s3.amazonaws.com, which is default value of ._endpoing when you create boto3.client('s3'). Which do you prefer?

@woop
Copy link
Member

woop commented Nov 17, 2020

allow_no_value

I prefer allow_no_value=True. Thanks for giving us options 💯

@mike0sv
S3 endpoint configuration feast-dev#1169
39cac68 
Signed-off-by: mike0sv <mike0sv@gmail.com>
@mike0sv mike0sv closed this Nov 17, 2020
@mike0sv
Add allow_no_value=True to ConfigParser
01def7f 
Signed-off-by: mike0sv <mike0sv@gmail.com>
@mike0sv
Copy link
Contributor Author

mike0sv commented Nov 17, 2020

Not sure, why this was closed and not sure why tests failed (seems to be not related to my changes). Please send help :)

@mike0sv mike0sv reopened this Nov 17, 2020
@woop
Copy link
Member

woop commented Nov 17, 2020

Not sure, why this was closed and not sure why tests failed (seems to be not related to my changes). Please send help :)

Can you please run make format prior to pushing your commit? That should fix lint-python

@pyalex is trying to fix the other test here: https://github.com/feast-dev/feast/pull/1173/files

@mike0sv
Copy link
Contributor Author

mike0sv commented Nov 17, 2020

lint-python fails because of the None, that changed DEAFULTS type from Dict[str, str] to Dict[str, Optional[str]] and mypy raises an error for deafults argument for ConfigParser. Not sure where it gets that it should be Mapping[str, str] thou

@mike0sv
Copy link
Contributor Author

mike0sv commented Nov 21, 2020

Unfortunately, the vars solution was not working, because vars takes precedence before the actual config, so passing DEFAULTS there were never gonna work. After that I tried to utilize fallback option, but this way I lost free type casting the configparser module provided. So I decided to approach the "defaults are saved to file" problem by hacking _defaults attribute for the time of writing. Added some tests and it seems to work

@mike0sv
oooopsie
2684151 
Signed-off-by: mike0sv <mike0sv@gmail.com>
@mike0sv
Copy link
Contributor Author

mike0sv commented Nov 21, 2020

/retest

woop
woop reviewed Nov 21, 2020
View reviewed changes
sdk/python/feast/config.py Outdated
return self._config.get(
CONFIG_FILE_SECTION,
option,
vars={**_get_feast_env_vars(), **self._options},
vars={
Copy link
Member

Choose a reason for hiding this comment

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

By the way, I think you should submit this change as part of a separate PR please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created new PR, it seems it needs to be merged before this one

@mike0sv
remove DEFAULTS logic changes
1f181f7 
Signed-off-by: mike0sv <mike0sv@gmail.com>
@mike0sv mike0sv closed this Nov 22, 2020
@mike0sv
Copy link
Contributor Author

mike0sv commented Nov 22, 2020

...

@mike0sv mike0sv reopened this Nov 22, 2020
@mike0sv
reformat
8e7dab4 
Signed-off-by: mike0sv <mike0sv@gmail.com>
@woop
Copy link
Member

woop commented Nov 22, 2020

Unfortunately, the vars solution was not working, because vars takes precedence before the actual config, so passing DEFAULTS there were never gonna work. After that I tried to utilize fallback option, but this way I lost free type casting the configparser module provided. So I decided to approach the "defaults are saved to file" problem by hacking _defaults attribute for the time of writing. Added some tests and it seems to work

Noted, didn't realize that.

This was referenced Nov 22, 2020
Closed
Merged
woop
woop reviewed Nov 22, 2020
View reviewed changes
sdk/python/feast/loaders/ingest.py
@@ -166,7 +167,7 @@ def _read_table_from_source(


def _upload_to_file_source(
file_url: str, with_partitions: bool, dest_path: str
file_url: str, with_partitions: bool, dest_path: str, config: Config
Copy link
Member

Choose a reason for hiding this comment

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

I think the arguments in the docstring is out of date. Would you mind updating?

@mike0sv
_upload_to_file_source docs
081288d 
Signed-off-by: mike0sv <mike0sv@gmail.com>
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mike0sv, 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 Nov 22, 2020

/lgtm

@feast-ci-bot feast-ci-bot merged commit aed366b into feast-dev:master Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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

@woop woop

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

Successfully merging this pull request may close these issues.

S3 endpoint configuration
3 participants
@mike0sv @feast-ci-bot @woop