Added support to anyopen for taking filelike objects; remove broken `.zip` support #197

dotsdl · 2022-06-08T05:15:31Z

Use in this way requires specifying the compression, if any, that the stream features in order to decompress it.

codecov · 2022-06-08T05:19:55Z

Codecov Report

Merging #197 (a036582) into master (e2f8c39) will decrease coverage by 0.05%.
The diff coverage is 94.73%.

❗ Current head a036582 differs from pull request most recent head 8240e64. Consider uploading reports for the commit 8240e64 to get more accurate results

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   98.20%   98.15%   -0.06%     
==========================================
  Files          24       24              
  Lines        1340     1352      +12     
  Branches      290      295       +5     
==========================================
+ Hits         1316     1327      +11     
  Misses          5        5              
- Partials       19       20       +1

Impacted Files	Coverage Δ
src/alchemlyb/parsing/util.py	`96.55% <94.73%> (-3.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2f8c39...8240e64. Read the comment docs.

dwhswenson

Thanks! It looks like this will largely work for OpenFE's needs. I've tested it out with our in-progress storage, and we'll just have to be sure we handle encodings on our end when that is needed.

A couple minor comments/suggestions from looking over the code.

dwhswenson · 2022-06-08T16:55:29Z

src/alchemlyb/parsing/util.py

+                    'zip': zipfile.ZipFile}
+
+    # if `datafile` is a stream
+    if hasattr(datafile, 'read'):


This should probably also explicitly consider the mode. (At least need to check if it has write for writing modes.)

So far anyopen hasn't been used with writing in mind in alchemlyb, but there's no reason it couldn't be I suppose. Added explicit checks for certain modes, and tests for roundtripping through a file and a stream.

dwhswenson · 2022-06-08T17:02:55Z

src/alchemlyb/parsing/util.py

+    compression : str
+        Use to specify compression.
+        Must be one of 'bz2', 'gz', 'zip'.


Currently, if pathlike, the filename's extension overrides the explicitly given compression here, which seems odd (although a conflict between compression algo and filename is a sign of something having gone seriously wrong).

Whether that stays as-is or not, the docstring here should clarify what happens if compression is given with a pathlike as datafile.

Ah, good catch. I've changed the behavior such that compression, if specified, always forces the compression assumed by anyopen.

dwhswenson · 2022-06-08T17:04:39Z

src/alchemlyb/parsing/util.py

+    # compression selections available
+    compressions = {'bz2': bz2_open,
+                    'gz': gzip_open,
+                    'zip': zipfile.ZipFile}


Can we safely assume that the options here will always correspond to the extensions, just without the .? If so, maybe it would be better to just do something like compression = "." + compression and use the extensions list?

Adjusted compression dict to make things more explicit, in which we specify either bzip2 or gzip for compression.

dwhswenson · 2022-06-08T17:08:54Z

src/alchemlyb/tests/parsing/test_util.py

@@ -12,3 +14,42 @@ def test_gzip():
        for filename in dataset['data'][leg]:
            with anyopen(filename, 'r') as f:
                assert type(f.readline()) is str
+


related to another comment, but maybe add a test for what happens when compression is given with a pathlike, just to ensure no API breaks on whatever behavior is decided on.

Added explicit test forcing compression despite wrong extension.

We never had working zip archive support from the looks of it. This is because zipfiles are not just a compression scheme, they are an archive format that features compression.

… detection

dotsdl · 2022-06-10T05:43:07Z

Thank you @dwhswenson for the review! I've implemented changes in response to your feedback.

orbeckst

Thanks for the upgrade. Small comments on docs. Coverage indicates that there's still quite a bit uncovered code, please increase testing. Please add an entry to CHANGES.

src/alchemlyb/parsing/util.py

orbeckst · 2022-06-27T23:52:37Z

@dotsdl any chance you can come back to the PR and address remaining comments? Would be nice to get it in.

dotsdl · 2022-06-28T00:48:57Z

@orbeckst yes, apologies, was on vacation through middle of last week and looping back to this shortly. Thank you for your review!

orbeckst

Please add a test for the case where the wrong compression is set on a stream. Otherwise I am very happy with everything — thank you @dotsdl !

src/alchemlyb/parsing/util.py

orbeckst · 2022-06-30T01:08:46Z

I'll get #200 fixed first and then re-run CI here.

orbeckst

Thank you for addressing my remaining comment. All good! Thanks!

dotsdl · 2022-06-30T01:13:15Z

@orbeckst danke! 🙏

orbeckst · 2022-06-30T02:06:37Z

(The checks had passed before I added everything to CHANGES and I because I need to get some supper, I took a short cut...)

Added support to anyopen for taking filelike objects

8fdd9bc

Use in this way requires specifying the compression, if any, that the stream features in order to decompress it.

Support python < 3.10

682ebc0

dwhswenson reviewed Jun 8, 2022

View reviewed changes

dotsdl changed the title ~~Added support to anyopen for taking filelike objects~~ Added support to anyopen for taking filelike objects; remove broken .zip support Jun 10, 2022

dotsdl added 3 commits June 9, 2022 22:16

Added checks for stream behavior in anyopen; dropped broken zip support

5f6e46d

We never had working zip archive support from the looks of it. This is because zipfiles are not just a compression scheme, they are an archive format that features compression.

Reworked anyopen to allow compression keyword to override extension…

5980a94

… detection

Added additional tests, explicit compression options for anyopen

23f86ee

orbeckst requested changes Jun 10, 2022

View reviewed changes

src/alchemlyb/parsing/util.py Show resolved Hide resolved

src/alchemlyb/parsing/util.py Show resolved Hide resolved

src/alchemlyb/parsing/util.py Show resolved Hide resolved

src/alchemlyb/parsing/util.py Show resolved Hide resolved

dotsdl added this to the 0.7.0 milestone Jun 29, 2022

dotsdl added 2 commits June 28, 2022 21:15

Updates from @orbeckst review

db5a9f7

Merge branch 'master' into anyopen-stream

a6b0636

orbeckst requested changes Jun 30, 2022

View reviewed changes

src/alchemlyb/parsing/util.py Show resolved Hide resolved

dotsdl mentioned this pull request Jun 30, 2022

[CI] account for changed pandas.concat sorting behavior #201

Merged

Added test for unsupported compression used for anyopen

dab5309

orbeckst approved these changes Jun 30, 2022

View reviewed changes

orbeckst added 2 commits June 29, 2022 18:14

Merge branch 'master' into anyopen-stream

a036582

update CHANGES

8240e64

orbeckst merged commit 72622c9 into master Jun 30, 2022

orbeckst deleted the anyopen-stream branch June 30, 2022 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support to anyopen for taking filelike objects; remove broken `.zip` support #197

Added support to anyopen for taking filelike objects; remove broken `.zip` support #197

dotsdl commented Jun 8, 2022

codecov bot commented Jun 8, 2022 •

edited

Loading

dwhswenson left a comment

dwhswenson Jun 8, 2022

dotsdl Jun 10, 2022

dwhswenson Jun 8, 2022

dotsdl Jun 10, 2022

dwhswenson Jun 8, 2022

dotsdl Jun 10, 2022

dwhswenson Jun 8, 2022

dotsdl Jun 10, 2022

dotsdl commented Jun 10, 2022

orbeckst left a comment

orbeckst commented Jun 27, 2022

dotsdl commented Jun 28, 2022

orbeckst left a comment

orbeckst commented Jun 30, 2022

orbeckst left a comment

dotsdl commented Jun 30, 2022

orbeckst commented Jun 30, 2022

Added support to anyopen for taking filelike objects; remove broken .zip support #197

Added support to anyopen for taking filelike objects; remove broken .zip support #197

Conversation

dotsdl commented Jun 8, 2022

codecov bot commented Jun 8, 2022 • edited Loading

Codecov Report

dwhswenson left a 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

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

dotsdl commented Jun 10, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jun 27, 2022

dotsdl commented Jun 28, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jun 30, 2022

orbeckst left a comment

Choose a reason for hiding this comment

dotsdl commented Jun 30, 2022

orbeckst commented Jun 30, 2022

Added support to anyopen for taking filelike objects; remove broken `.zip` support #197

Added support to anyopen for taking filelike objects; remove broken `.zip` support #197

codecov bot commented Jun 8, 2022 •

edited

Loading