Fix s3_put_object retries in datastore/s3.py. Also fail gracefully if… #234
Conversation
… boto3 not installed in datatools Uploads were not retried correctly since the buffer status was not reset after the first attempt which led to an "IO on closed file" error or an empty file being written to S3
|
Also added @tuulos who was the original author of this fix (internally) IIRC. |
metaflow/datastore/s3.py
Outdated
|
|
||
| from .datastore import MetaflowDataStore, DataException, only_if_not_done | ||
| from ..metadata import MetaDatum | ||
| from .util.s3util import aws_retry, get_s3_client | ||
| from .. import metaflow_config |
There was a problem hiding this comment.
Can we move this above L:20?
While on this subject do you know if there is a special format that python auto-format tools follow for organizing import statements (alphabetical, relative references last etc.)?
I usually follow alphabetical, and prefer absolute imports first then relative (like from .metadata import foo)
There was a problem hiding this comment.
There is usually a preference. I think it is system ones first then user ones. Both in alphabetical order. Not sure about absolute vs relatives. I think once we run the auto format tools, this should reformat everything appropriately but can definitely move this one above if you want.
metaflow/datatools/s3.py
Outdated
| @@ -197,6 +203,9 @@ def __init__(self, | |||
| tmproot: (optional) Root path for temporary files (default: '.') | |||
| """ | |||
|
|
|||
| if not boto_found: | |||
| MetaflowException("You need to install 'boto3' in order to use S3.") | |||
There was a problem hiding this comment.
Are you missing a raise here?
There was a problem hiding this comment.
Definitely. Good catch.
|
lgtm |
|
Superseded by #288 |
… boto3 not installed in datatools
Uploads were not retried correctly since the buffer status was
not reset after the first attempt which led to an "IO on closed file"
error or an empty file being written to S3