-
Notifications
You must be signed in to change notification settings - Fork 363
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.
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
Delete the downloaded zip and folder in retrieve_dataset
#2150
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Gaurav Gupta <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2150 +/- ##
==========================================
+ Coverage 91.02% 92.21% +1.19%
==========================================
Files 115 103 -12
Lines 6058 5141 -917
==========================================
- Hits 5514 4741 -773
+ Misses 544 400 -144
Flags with carried forward coverage won't be shown. Click here to find out more.
|
import uuid | ||
|
||
DOWNLOADED_DATASET_DIR = 'datasets.4.27.2021' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems oddly specific. Isn't it going to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use a specific zip folder with the date it was created for these datasets (that are put on our blob storage for tests)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so if the dataset changes this need to be updated. How will we remember?
@@ -29,7 +32,7 @@ def retrieve_dataset(dataset, **kwargs): | |||
:rtype: object | |||
""" | |||
# if data not extracted, download zip and extract | |||
outdirname = 'datasets.4.27.2021' | |||
outdirname = DOWNLOADED_DATASET_DIR | |||
if not os.path.exists(outdirname): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the dataset is downloaded we don't re-download it here... we just re-use the downloaded one
else: | ||
raise Exception('Unrecognized file extension: ' + extension) | ||
|
||
shutil.rmtree(outdirname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this increase network calls and also chance tests fail due to networking issues? also I think this might increase test time a lot? we currently just re-use the downloaded file in all test cases instead of re-downloading it every time
Description
Delete the downloaded zip and folder in
retrieve_dataset
Checklist