VR-9892: Unzip directory artifacts on download#1973
Conversation
| zipf.extractall(dirpath) | ||
| finally: | ||
| os.remove(temp_filepath) | ||
| print("download complete; directory extracted to {}".format(dirpath)) |
There was a problem hiding this comment.
With your approval @convoliution I'll make a tech-debt ticket to evaluate logging practices in our client library and plan for improvement. I'm pretty sure that in the longer term we want to steer away from using print statements so that our users can do things like choose their own log level. There's a good starting point here in the Hitchhiker's Guide to Python which we can use as a jumping off point.
There was a problem hiding this comment.
@daniel-verta Certainly! I'd be more than thrilled to move nearly the entirety of the client's print statements to a logger, provided it's done consistently.
There was a problem hiding this comment.
I believe we have received such request, actually.
|
|
||
|
|
||
| def download(response, filepath, chunk_size=32*(10**6), overwrite_ok=False): | ||
| def download_response(response, chunk_size=32*(10**6)): |
There was a problem hiding this comment.
Can this default value become an informative named constant?
There was a problem hiding this comment.
Would _32MB be informative, or would you be thinking of something that explains why that number was chosen (there wasn't a particular reason 😅).
There was a problem hiding this comment.
_32MB is fine. If there was a specific motivation for that number then a comment would be helpful. Just a rename in this case is helpful.
There was a problem hiding this comment.
I think this idea makes sense, and there are also different places (and different values...) where N*(10**6) is used. I'm going to consolidate them and make that a separate PR shortly.
conradoverta
left a comment
conradoverta
left a comment
There was a problem hiding this comment.
LGTM, but need to address Daniel's feedback.
3 participants
.dir.zipextension.dir.zipextensionrun.download_model()