Add attributes to stream types needed by TextIOWrapper #192
Conversation
|
See aiidateam/aiida-core#6847 for the related PR in aiida-core. |
…with IO stream API from Python Standard Library
|
I've added some tests for the new attributes. |
|
Also, I now added these attributes to the stream types that I believe cover the aiida use cases, but I don't know if it makes sense to add them as well to the other stream types in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #192 +/- ##
=======================================
Coverage 99.66% 99.67%
=======================================
Files 10 10
Lines 2116 2143 +27
=======================================
+ Hits 2109 2136 +27
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hi @ahkole
Thanks a lot for the nice implementation, to me it looks good and reasonable.
I have a few comments on the tests.
Please have a look, once addressed this PR can be merged.
P.S.
I apologize for the delay, I was in a vacation 🥲
| def test_packed_object_reader_readable(): | ||
| """Test the ``PackedObjectReader.readable`` function.""" | ||
| with tempfile.TemporaryFile() as handle: | ||
| reader = utils.PackedObjectReader(handle, 0, 0) | ||
|
|
||
| assert reader.readable() | ||
|
|
||
|
|
||
| def test_packed_object_reader_writable(): | ||
| """Test the ``PackedObjectReader.writable`` function.""" | ||
| with tempfile.TemporaryFile() as handle: | ||
| reader = utils.PackedObjectReader(handle, 0, 0) | ||
|
|
||
| assert not reader.writable() | ||
|
|
||
|
|
||
| def test_packed_object_reader_closed(): | ||
| """Test the ``PackedObjectReader.closed`` property.""" | ||
| with tempfile.TemporaryFile() as handle: | ||
| reader = utils.PackedObjectReader(handle, 0, 0) | ||
|
|
||
| assert not reader.closed | ||
| assert reader.closed |
There was a problem hiding this comment.
I think you can merge this into test_packed_object_reader_mode just put your asserts in the end of that function, and maybe modify the test description.
| temp_container.pack_all_loose(compress=True) | ||
| temp_container.clean_storage() | ||
|
|
||
| # LazyLooseStream will never be opened |
There was a problem hiding this comment.
If I understand correctly, you want to test here the situation that if LazyLooseStream is in use, but not opened, that should not have a conflict with your method of actually opening an stream, is that right?
In that case, I'd add merge test with around lines 3477, just after:
assert not loosepath.exists()
There was a problem hiding this comment.
What I wanted to test here was specifically the closed property (which I added) for the case where you opened a ZlibLikeBaseStreamDecompresser that has an internal LazyLooseStream but where you don't wind back the file pointer so that the LazyLooseStream is actually never opened. This was just intended to cover all the bases/situations of the ZlibLikeBaseStreamDecompresser but not sure if this tests adds much or makes sense.
I'm not entirely sure btw what you meant with this:
the situation that if LazyLooseStream is in use, but not opened, that should not have a conflict with your method of actually opening an stream
What are you referring to with 'your method of opening an stream' and what kind of conflict could be there?
There was a problem hiding this comment.
I'm not entirely sure btw what you meant with this:
The same thing as you mentioned. I think this test make sense.
However, since the setup is similar, for more readability I'd suggest to move your test here, between:
disk-objectstore/tests/test_container.py
Line 3477 in e8df2f3
and
disk-objectstore/tests/test_container.py
Line 3481 in e8df2f3
| for _, stream, _ in triplets: | ||
| assert isinstance(stream, utils.ZlibLikeBaseStreamDecompresser) | ||
| assert not stream.closed | ||
| assert stream.closed |
There was a problem hiding this comment.
How many stream will be there? and why to assert stream.closed only the last one?
There was a problem hiding this comment.
If I understand get_objects_stream_and_meta correctly, there will be one triplet for each hashkey in the list of arguments. Since I am supplying a single hashkey here I would expect there to only be a single stream. Hence, there is only one to check for closure after the with block. Maybe not the most easy to understand approach, but this was the only routine I could find so far that has a ZlibLikeBaseStreamDecompresser with an internal LazyLooseStream.
There was a problem hiding this comment.
Thanks for the explanation I think it's clear now.
I suggest to get rid of the for loop, as there's only one triplet in the list:
| for _, stream, _ in triplets: | |
| assert isinstance(stream, utils.ZlibLikeBaseStreamDecompresser) | |
| assert not stream.closed | |
| assert stream.closed | |
| assert len(triplets) == 1 | |
| stream = triplets[0][1] | |
| assert isinstance(stream, utils.ZlibLikeBaseStreamDecompresser) | |
| assert not stream.closed | |
| assert stream.closed |
|
Hi @khsrali No worries about the delay. Thanks for having a look at the PR and for your comments. I'm currently busy preparing for a workshop. I'll reply quickly to your comments where I can. The rest I will address after the workshop. |
These changes are needed in order to wrap the streams in a
TextIOWrapperfor streaming access to files in text-mode in the disk-objectstore. A related PR is opened inaiida-corethat uses this to provide streaming access in text-mode to files in the aiida repository.Based on my testing, only
PackedObjectReaderis actually used in practice in my use-case of aiida, but the pre-commit hooks required the properties to be added as well toCallbackStreamWrapperandZlibLikeBaseStreamDecompresserdue to a type union. Also, based on my testing these three properties are the minimal set that was still missing for use withTextIOWrapper.