Remove cloud storage from Python package #2310
Conversation
|
/test integration |
|
Fri Aug 21 11:16:30 UTC 2020 impatient try |
|
Fri Aug 21 11:16:34 UTC 2020 impatient try |
|
Fri Aug 21 11:16:40 UTC 2020 impatient try |
|
Fri Aug 21 11:16:55 UTC 2020 impatient try |
|
/test integration |
|
/cc @axsaucedo @cliveseldon @RafalSkolasinski |
|
Fri Aug 21 11:23:03 UTC 2020 impatient try |
|
Fri Aug 21 11:23:05 UTC 2020 impatient try |
|
Fri Aug 21 11:23:10 UTC 2020 impatient try |
|
Fri Aug 21 11:25:19 UTC 2020 impatient try |
|
Fri Aug 21 12:46:14 UTC 2020 impatient try |
|
Fri Aug 21 12:46:21 UTC 2020 impatient try |
|
/test integration |
|
Fri Aug 21 14:43:20 UTC 2020 impatient try |
|
This looks great and will unblock us to upgrade to the latest mlflow! Thanks! |
|
@anggao: changing LGTM is restricted to collaborators In response to this:
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 jenkins-x/lighthouse repository. |
|
/retest |
|
@anggao the plan is to release this in version |
|
Fri Aug 21 16:50:55 UTC 2020 impatient try |
|
@adriangonz Thanks for the info, is it possible to patch release this if |
|
/test notebooks |
|
Mon Aug 24 07:59:54 UTC 2020 impatient try |
|
@adriangonz looks good in general, although I just noticed that the storage.py dependency wasn't removed from the prepackaged models (not sure how it's passing the integration tests) - may be worth ensuring that the prepackaged servers no longer depend on the storage.py library. Another thing that would be important is to add an entry on the UPGRADING.md about the breaking change (in case anyone is using the storagepy) and perhaps the alternative of installing the kfserving component (unless there is an alternative we want to explore) |
|
@axsaucedo that's described above. I've kept the same |
|
Thanks for confirming - makes sense, good shout. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anggao, axsaucedo 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 |
|
Mon Aug 24 15:57:38 UTC 2020 impatient try |
|
Mon Aug 24 15:58:11 UTC 2020 impatient try |
What this PR does / why we need it:
Removes cloud storage integrations (e.g. S3, GCS, Azure) from the
seldon_corePython package. The current implementation interacts with these services through a separate image coming from the KFServing project, so they are not needed in theseldon_corepackage anymore.In particular, whenever we need to fetch something remotely (e.g. setting
modelUri: s3://...inSeldonDeploymentresources), this separate image will do that through an init container. After that's downloaded, the only thing that theseldon_corewrapper will need to do is copy these resources into a local path.This PR also removes the
requirements.txtfile so thatsetup.pyis now the single source-of-truth for dependencies. Note that we still have arequirements-dev.txtfile there, to list dependencies for testing, linting, etc.Which issue(s) this PR fixes:
Fixes #2140
Fixes #1371
Special notes for your reviewer:
I've still kept the
storage.pyfile and the sameStorage.download()interface to avoid changing the different places where we callStorage.download(). However, it will now only support the_download_local()method, which is the one copying over the files to a local path.Does this PR introduce a user-facing change?: