S3 endpoint configuration #1169 by mike0sv · Pull Request #1172 · feast-dev/feast

mike0sv · 2020-11-16T16:40:46Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

Added an option to configure S3 endpoint url

feast-ci-bot · 2020-11-16T16:40:55Z

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.

Details

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-11-17T02:19:01Z

Thanks for this @mike0sv!

/ok-to-test

woop · 2020-11-17T02:19:12Z

/kind feature

mike0sv · 2020-11-17T04:51:11Z

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 · 2020-11-17T05:03:27Z

allow_no_value

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

Signed-off-by: mike0sv <mike0sv@gmail.com>

mike0sv · 2020-11-17T05:35:43Z

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

woop · 2020-11-17T05:56:33Z

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 · 2020-11-17T06:18:20Z

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 · 2020-11-21T06:22:51Z

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

Signed-off-by: mike0sv <mike0sv@gmail.com>

mike0sv · 2020-11-21T06:49:08Z

/retest

woop · 2020-11-21T10:17:32Z


        self._config = config  # type: ConfigParser
        self._path = path  # type: str



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

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

Signed-off-by: mike0sv <mike0sv@gmail.com>

mike0sv · 2020-11-22T02:30:09Z

...

Signed-off-by: mike0sv <mike0sv@gmail.com>

woop · 2020-11-22T03:04:37Z

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.

# Conflicts: # sdk/python/feast/staging/storage_client.py

woop · 2020-11-22T08:29:57Z


 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


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

Signed-off-by: mike0sv <mike0sv@gmail.com>

feast-ci-bot · 2020-11-22T09:12:34Z

[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

Details

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-11-22T09:12:39Z

/lgtm

mike0sv requested review from davidheryanto, khorshuheng, pyalex, woop and zhilingc as code owners November 16, 2020 16:40

feast-ci-bot added release-note needs-kind labels Nov 16, 2020

feast-ci-bot added needs-ok-to-test size/M labels Nov 16, 2020

feast-ci-bot added ok-to-test and removed needs-ok-to-test labels Nov 17, 2020

feast-ci-bot added kind/feature New feature or request and removed needs-kind labels Nov 17, 2020

S3 endpoint configuration feast-dev#1169

39cac68

Signed-off-by: mike0sv <mike0sv@gmail.com>

mike0sv closed this Nov 17, 2020

mike0sv force-pushed the master branch from 836d190 to fb46663 Compare November 17, 2020 05:24

feast-ci-bot added size/XS and removed size/M labels Nov 17, 2020

Add allow_no_value=True to ConfigParser

01def7f

Signed-off-by: mike0sv <mike0sv@gmail.com>

mike0sv reopened this Nov 17, 2020

feast-ci-bot added size/M and removed size/XS labels Nov 17, 2020

feast-ci-bot added size/L and removed size/M labels Nov 21, 2020

oooopsie

2684151

Signed-off-by: mike0sv <mike0sv@gmail.com>

woop reviewed Nov 21, 2020

View reviewed changes

remove DEFAULTS logic changes

1f181f7

Signed-off-by: mike0sv <mike0sv@gmail.com>

mike0sv closed this Nov 22, 2020

mike0sv force-pushed the master branch from 2684151 to 77a83fd Compare November 22, 2020 02:26

feast-ci-bot added size/XS and removed size/L labels Nov 22, 2020

mike0sv reopened this Nov 22, 2020

feast-ci-bot added size/M and removed size/XS labels Nov 22, 2020

reformat

8e7dab4

Signed-off-by: mike0sv <mike0sv@gmail.com>

This was referenced Nov 22, 2020

With defaults #1187

Closed

Dont write defaults to config #1188

Merged

mike0sv added 2 commits November 22, 2020 10:07

Merge branch 'master' into with_defaults

b94d109

Merge branch 'with_defaults' into master_mike0sv

61e2ce9

# Conflicts: # sdk/python/feast/staging/storage_client.py

woop reviewed Nov 22, 2020

View reviewed changes

_upload_to_file_source docs

081288d

Signed-off-by: mike0sv <mike0sv@gmail.com>

woop approved these changes Nov 22, 2020

View reviewed changes

feast-ci-bot added the approved label Nov 22, 2020

feast-ci-bot assigned woop Nov 22, 2020

feast-ci-bot added the lgtm label Nov 22, 2020

feast-ci-bot merged commit aed366b into feast-dev:master Nov 22, 2020


		self._config = config # type: ConfigParser
		self._path = path # type: str

Uh oh!

Conversation

mike0sv commented Nov 16, 2020

Uh oh!

feast-ci-bot commented Nov 16, 2020

Uh oh!

woop commented Nov 17, 2020

Uh oh!

woop commented Nov 17, 2020

Uh oh!

mike0sv commented Nov 17, 2020

Uh oh!

woop commented Nov 17, 2020

Uh oh!

mike0sv commented Nov 17, 2020

Uh oh!

woop commented Nov 17, 2020

Uh oh!

mike0sv commented Nov 17, 2020

Uh oh!

mike0sv commented Nov 21, 2020

Uh oh!

mike0sv commented Nov 21, 2020

Uh oh!

woop Nov 21, 2020

Choose a reason for hiding this comment

Uh oh!

mike0sv Nov 22, 2020

Choose a reason for hiding this comment

Uh oh!

mike0sv commented Nov 22, 2020

Uh oh!

woop commented Nov 22, 2020

Uh oh!

woop Nov 22, 2020

Choose a reason for hiding this comment

Uh oh!

feast-ci-bot commented Nov 22, 2020

Uh oh!

woop commented Nov 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants