Fix ArrowWriter closes stream at exit #1971

albertvillanova · 2021-03-02T07:12:34Z

Current implementation of ArrowWriter does not properly release the stream resource (by closing it) if its finalize() method is not called and/or an Exception is raised before/during the call to its finalize() method.

Therefore, ArrowWriter should be used as a context manager that properly closes its stream resource at exit.

This reverts commit b28967f.

albertvillanova · 2021-03-02T11:04:22Z

src/datasets/metric.py

@@ -336,7 +336,8 @@ def _finalize(self):
        from all the nodes if main node or all_process is True.
        """
        if self.writer is not None:
-            self.writer.finalize()
+            with self.writer as writer:
+                writer.finalize()
        self.writer = None


Hi @lhoestq , do you think this is the cause of the test error? I am trying to understand why... It happens with DistributedDatasetTest.test_load_dataset_distributed. Maybe you know why...

lhoestq · 2021-03-02T14:38:23Z

Oh nice thanks for adding the context manager ! All the streams and RecordBatchWriter will be properly closed now. Hopefully this gives a better experience on windows on which it's super important to close stuff.

Not sure about the error, it looks like a process crashed silently.
Let me take a look

albertvillanova · 2021-03-02T14:54:32Z

Hopefully this gives a better experience on windows on which it's super important to close stuff.

Exactly! On Windows, you got:

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process

when trying to access the unclosed stream file, e.g. by with incomplete_dir(self._cache_dir) as tmp_data_dir: shutil.rmtree(tmp_dir)

The reason is: https://docs.python.org/3/library/os.html#os.remove

On Windows, attempting to remove a file that is in use causes an exception to be raised; on Unix, the directory entry is removed but the storage allocated to the file is not made available until the original file is no longer in use.

lhoestq · 2021-03-02T14:55:40Z

The test passes on my windows. This was probably a circleCI issue. I re-ran the circleCI tests

albertvillanova · 2021-03-02T14:56:29Z

NICE! It passed!

lhoestq

Really cool thanks :)

I just have one comment:

lhoestq · 2021-03-02T15:11:48Z

src/datasets/metric.py

@@ -336,7 +336,8 @@ def _finalize(self):
        from all the nodes if main node or all_process is True.
        """
        if self.writer is not None:
-            self.writer.finalize()
+            with self.writer as writer:
+                writer.finalize()


The usage of the with statement here can make the user think that the writer opens and creates a file here.
This is not the case since finalize just writes down the latest examples and close the file.

It would be prettier to use .close() instead of a with statement here, to remove this ambiguity.

The with statement should be used to define the start and end point of the usage of the writer (file opened until closed).
However in this case the writer is an attribute of the Metric object and so there's no code block with a defined start/end.
In this case I think it's better to use .close() instead. You can even do try/except to make sure that close() is called even if finalize fails.

It would makes things clearer IMO.

@lhoestq, I think the main point is in your sentence:

You can even do try/except to make sure that close() is called even if finalize fails.

Exactly! This is why I decided to open this PR: an exception was raised, and the resource was not closed. If an exception is raised within .finalize(), the file will no be closed. So that the solution would be: try... except... finally: close().

And that is exactly the use case for with: use a context manager instead of try... except... finally: close().
See: https://docs.python.org/3/reference/compound_stmts.html#the-with-statement

This allows common try…except…finally usage patterns to be encapsulated for convenient reuse.

In this case maybe you can you add in the writer's docstring that the with can also be used only when calling finalize ?
As a user I'd expect the with statement of a Writer class to be necessarily used from start of writing till closing, but here you can write and then use the with statement to close. Let me know if that makes sense.

That's definitely not a big deal but I'd rather have things said explicitly :)

Yes @lhoestq, you are right. One should use the context manager from start of writing until closing. I left some usages of writer without context manager, because the refactoring was not immediate, for a follow-up PR. Maybe I can remove the with in this case, to finish this PR. And then open another PR for the remaining non-trivial cases?

Sounds good !

This reverts commit 7f287fb.

lhoestq · 2021-03-10T10:31:41Z

Maybe you can merge master into this branch and check the CI before merging ?

albertvillanova · 2021-03-10T16:04:26Z

@lhoestq done! ;)

lhoestq · 2021-03-10T16:36:50Z

Thanks ! merging

albertvillanova added 15 commits March 2, 2021 08:06

Test ArrowWriter closes stream at exit

49d14fb

Implement ArrowWriter as context manager and close stream at exit

d3c0e0b

Specify exception class

19cd459

Use ArrowWriter as context manager

b28967f

Fix ArrowWriter.close

8c230ee

Merge remote-tracking branch 'upstream/master' into fix-arrow-writer

4d50e95

Fix ArrowWriter.close

2a7a861

Test stream already closed at ArrowWriter context exit

5c7af22

Revert "Use ArrowWriter as context manager"

ba368f5

This reverts commit b28967f.

Use ArrowWriter as context manager in tests

d2e123d

Generalize Exception type in ArrowWriter.close

84ad7f3

Use ArrowWriter as context manager in benchmarks

c521ffe

Use ArrowWriter as context manager in arrow_writer and builder

7d241ce

Use ArrowWriter as context manager in arrow_dataset

e00d72a

Use ArrowWriter as context manager in metric

7f287fb

albertvillanova commented Mar 2, 2021

View reviewed changes

lhoestq reviewed Mar 2, 2021

View reviewed changes

Revert "Use ArrowWriter as context manager in metric"

10d9258

This reverts commit 7f287fb.

lhoestq approved these changes Mar 10, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-arrow-writer

6f1a701

lhoestq merged commit 13a5b7d into huggingface:master Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ArrowWriter closes stream at exit #1971

Fix ArrowWriter closes stream at exit #1971

albertvillanova commented Mar 2, 2021

albertvillanova Mar 2, 2021

lhoestq commented Mar 2, 2021

albertvillanova commented Mar 2, 2021 •

edited

Loading

lhoestq commented Mar 2, 2021

albertvillanova commented Mar 2, 2021

lhoestq left a comment

lhoestq Mar 2, 2021

albertvillanova Mar 9, 2021 •

edited

Loading

lhoestq Mar 9, 2021

albertvillanova Mar 9, 2021

lhoestq Mar 10, 2021

lhoestq commented Mar 10, 2021

albertvillanova commented Mar 10, 2021

lhoestq commented Mar 10, 2021

Fix ArrowWriter closes stream at exit #1971

Fix ArrowWriter closes stream at exit #1971

Conversation

albertvillanova commented Mar 2, 2021

albertvillanova Mar 2, 2021

Choose a reason for hiding this comment

lhoestq commented Mar 2, 2021

albertvillanova commented Mar 2, 2021 • edited Loading

lhoestq commented Mar 2, 2021

albertvillanova commented Mar 2, 2021

lhoestq left a comment

Choose a reason for hiding this comment

lhoestq Mar 2, 2021

Choose a reason for hiding this comment

albertvillanova Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

lhoestq Mar 9, 2021

Choose a reason for hiding this comment

albertvillanova Mar 9, 2021

Choose a reason for hiding this comment

lhoestq Mar 10, 2021

Choose a reason for hiding this comment

lhoestq commented Mar 10, 2021

albertvillanova commented Mar 10, 2021

lhoestq commented Mar 10, 2021

albertvillanova commented Mar 2, 2021 •

edited

Loading

albertvillanova Mar 9, 2021 •

edited

Loading