Add multipart zip support #62

iranroman · 2021-06-04T05:39:49Z

This PR implements handling of multi-part zip files, which was the zip format of the fsd50k dataset and resulted in #55 . This will be needed to write the loaders for TAU NIGENS SSE 2021, TAU NIGENS SSE 2020, and TAU SSE 2019.

genisplaja · 2021-06-04T23:47:39Z

soundata/download_utils.py

+                zip_path = save_dir+"/"+k+".zip"
+                outpath = save_dir+"/"+k+"_single.zip"


Maybe using os.path.join() here?

genisplaja · 2021-06-04T23:58:52Z

Thanks @iranroman for that!:)

Regarding your last comment on #55: yes of course it has no sense for the user to get just some data splits instead of downloading them all. In my version of the FSD50K loader, I added Exceptions to make sure that users were not allowed to download just a single split alone but the entire development or evaluation splits. However, I like the idea of storing the RemoteFileMetadata of the splits as a list to identify the multipart zips, and then using the expected final folder name as key.

However, there is something I don't understand: where are we testing that merging and unzipping code now? Should this be done in test_download_utils.py?

iranroman · 2021-06-05T02:16:52Z

Hello @genisplaja. Thanks for your feedback.

However, there is something I don't understand: where are we testing that merging and unzipping code now? Should this be done in test_download_utils.py?

Isn't it implied that the download, merging, and unzipping all were done correctly when the pytest -s tests/test_full_dataset.py --local --dataset fsd50 passes?

iranroman · 2021-06-05T07:45:18Z

Hello @genisplaja. I followed your suggestion and I have added a multipart zip test in test_download_utils.py. I also refactored the code to create a download_multipart_zip function in download_utils.py. Let me know if you have more suggestions.

genisplaja · 2021-06-05T17:28:36Z

Looks good @iranroman!! Thanks for that:) You need to make sure that every line is covered by tests, even if the test_full_dataset passes in local! Codecov is checking that for you every time you update branch in GitHub. Any idea why the circleCI tests are not passing here? I cloned your repo and all tests all passing locally for me. Have you done black formatting?

iranroman · 2021-06-05T18:00:10Z

No idea @genisplaja. This is my first PR int his repo. I was expecting them to just pass. You would know better than me.

…data into add_multipart_zip_support

codecov · 2021-10-13T22:23:56Z

Codecov Report

Merging #62 (15782d9) into master (04b4a01) will decrease coverage by 0.03%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
- Coverage   97.91%   97.87%   -0.04%     
==========================================
  Files          18       18              
  Lines        1437     1415      -22     
==========================================
- Hits         1407     1385      -22     
  Misses         30       30

iranroman · 2021-10-14T18:39:43Z

Hello all, codecov is complaining about lines that I did not originally write. What should I do to make codecov ignore these lines as before?

magdalenafuentes · 2021-10-14T21:58:36Z

@iranroman will look into this tomorrow!

…data into add_multipart_zip_support

iranroman · 2021-10-18T21:24:05Z

Hello @genisplaja.

This PR is very relevant to the dataset loader you wrote for fsd50k. Would you like to have a look before we close it?

magdalenafuentes · 2021-10-18T21:22:14Z

soundata/download_utils.py

@@ -54,6 +55,8 @@ def downloader(
            The directory to download the data
        remotes (dict or None):
            A dictionary of RemoteFileMetadata tuples of data in zip format.
+            If a dictionary key has a list of RemoteFileMetadata,


nit: Maybe rephrase? If an element of the dictionary is a list of RemoteFileMetadata?

makes sense. Done

magdalenafuentes · 2021-10-18T21:26:19Z

soundata/download_utils.py


-            if remotes[k].unpack_directories:
-                for src_dir in remotes[k].unpack_directories:
+            if isinstance(remotes[k], list):


Since you're assuming that the multiple files are zip, maybe you can check that and add an NotImplementedError if it is not?

Thanks for this idea. I've implemented it

genisplaja · 2021-10-18T21:31:39Z

Yes! I'll do that tomorrow morning (it's almost midnight here!). Thanks for letting me know:)

iranroman · 2021-10-19T01:35:15Z

No problem @genisplaja.

In the meantime I ran pytest -s tests/test_full_dataset.py --local --dataset fsd50k and everything passed for me (including download, multipart joining, and unzipping).

The multipart zip download, joining, and unpacking now is handled by download_utils.py. Let us know your thoughts.

genisplaja · 2021-10-19T12:41:44Z

soundata/datasets/fsd50k.py

@@ -206,51 +206,54 @@
    primaryClass={cs.SD}
 }
 """
+
+# a dictionary key that hasa a list of RemoteFileMetadata implies a multi-part zip


typo: has a

genisplaja · 2021-10-19T12:56:38Z

soundata/download_utils.py

+    """
+    for l in range(len(zip_remotes)):
+        download_from_remote(zip_remotes[l], save_dir, force_overwrite)
+    zip_path = os.path.join(save_dir, obj_to_download + ".zip")


This is a good way to get the principal zip filename to use it as input of command zip, but it forces the use of the principal filename without the .zip extension as key of the list of parts of the split compressed file in the REMOTES dict (see line 213-214 of FSD50K loader). Am I right?
If so, this may cause confusion for an external contributor, since in other loaders one would just put, for instance, development as key, as I did for the original FSD50K loader. This should be specified somewhere in the docs, or I would suggest to iterate over the download item list parsing the field filename to identify the one with .zip extension, which is actually the input filename for the zip command. I think it would be something like:

zip_path = next((part.filename for part in zip_remotes if '.zip' in part.filename), None)

and then you can get the output path like:

out_path = zip_path.replace('.zip', '_single.zip')

Then the input obj_to_download would be no longer needed. Since at the moment this function is just for multipart files in zip format, it should work! And then the contributor is allowed to use any key for the divided file.
Also because I think that you don't actually check that the actual filename is k + '.zip', or obj_to_download + '.zip'.
Just a proposal:) What do you think?

Hello, thanks for your suggestions.

I'm afraid I tried making the changes you suggested and now the tests are not passing. do you wanna give it a shot and open a PR against this one before we join with main?

Never mind. I got it. Great suggestions, by the way. Thanks a lot!

Great, no worries! This PR looks ready to me.

iranroman added 2 commits June 3, 2021 22:14

implements multipart zip support

0e043a5

small edits

d1dc234

iranroman mentioned this pull request Jun 4, 2021

[fsd50k] make handling of multipart zip work across unix systems #55

Closed

genisplaja reviewed Jun 4, 2021

View reviewed changes

iranroman added 2 commits June 4, 2021 19:33

uses os.path.join() instead of concat strings

035f866

adds multipart zip in test_download_utils

be106d9

magdalenafuentes and others added 5 commits September 16, 2021 17:50

Merge branch 'master' into add_multipart_zip_support

4ba87ec

formatting fixes

1a0edef

Merge branch 'master' into add_multipart_zip_support

5ee13cb

black version 20.9b0 formatting

39c2138

Merge branch 'add_multipart_zip_support' of github.com:iranroman/soun…

a869c63

…data into add_multipart_zip_support

iranroman added 6 commits October 13, 2021 18:48

updates download_utils.py

2a91447

black formatting

9a1b44d

adds multipart_zip test

beb510c

fixes formatting

cb6823c

mutipart_zip test with cleanup

40b8a61

remove cleanup

556223f

updates download_multipart_zip test

327a75f

magdalenafuentes added a commit that referenced this pull request Oct 18, 2021

mimick change #62

f5e5d46

magdalenafuentes mentioned this pull request Oct 18, 2021

Add unpack directories tests #73

Merged

magdalenafuentes closed this in #73 Oct 18, 2021

magdalenafuentes added a commit that referenced this pull request Oct 18, 2021

mimick change #62

e695c33

magdalenafuentes reopened this Oct 18, 2021

iranroman added 15 commits October 18, 2021 17:14

Merge branch 'master' into add_multipart_zip_support

6862997

implements multipart zip support

f95b9db

small edits

e5d836a

uses os.path.join() instead of concat strings

cd1c5d0

adds multipart zip in test_download_utils

21a02bf

formatting fixes

a803d4a

black version 20.9b0 formatting

3e5e8ea

updates download_utils.py

846f70a

black formatting

2047420

adds multipart_zip test

b736439

fixes formatting

7864f3d

mutipart_zip test with cleanup

5827197

remove cleanup

a44c04b

updates download_multipart_zip test

235f26a

Merge branch 'add_multipart_zip_support' of github.com:iranroman/soun…

7887561

…data into add_multipart_zip_support

magdalenafuentes reviewed Oct 18, 2021

View reviewed changes

iranroman added 3 commits October 18, 2021 20:54

removes download and unzip test for fsd50k

14f8e7f

addresses comments by @magdalenafuentes

6ef28f7

formatting

01c9318

genisplaja reviewed Oct 19, 2021

View reviewed changes

hangles multipart zip by filename instead of obj_to_download

15782d9

magdalenafuentes approved these changes Oct 19, 2021

View reviewed changes

magdalenafuentes merged commit 978c185 into soundata:master Oct 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multipart zip support #62

Add multipart zip support #62

iranroman commented Jun 4, 2021

genisplaja Jun 4, 2021

iranroman Jun 5, 2021

genisplaja commented Jun 4, 2021

iranroman commented Jun 5, 2021

iranroman commented Jun 5, 2021

genisplaja commented Jun 5, 2021 •

edited

iranroman commented Jun 5, 2021

codecov bot commented Oct 13, 2021 •

edited

iranroman commented Oct 14, 2021

magdalenafuentes commented Oct 14, 2021

iranroman commented Oct 18, 2021

magdalenafuentes Oct 18, 2021

iranroman Oct 19, 2021

magdalenafuentes Oct 18, 2021

iranroman Oct 19, 2021

genisplaja commented Oct 18, 2021

iranroman commented Oct 19, 2021

genisplaja Oct 19, 2021

genisplaja Oct 19, 2021

iranroman Oct 19, 2021

iranroman Oct 19, 2021

genisplaja Oct 19, 2021

		zip_path = save_dir+"/"+k+".zip"
		outpath = save_dir+"/"+k+"_single.zip"

Add multipart zip support #62

Add multipart zip support #62

Conversation

iranroman commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genisplaja commented Jun 4, 2021

iranroman commented Jun 5, 2021

iranroman commented Jun 5, 2021

genisplaja commented Jun 5, 2021 • edited

iranroman commented Jun 5, 2021

codecov bot commented Oct 13, 2021 • edited

Codecov Report

iranroman commented Oct 14, 2021

magdalenafuentes commented Oct 14, 2021

iranroman commented Oct 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genisplaja commented Oct 18, 2021

iranroman commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genisplaja commented Jun 5, 2021 •

edited

codecov bot commented Oct 13, 2021 •

edited