Fix file-like closing bug from `gensim.corpora.MmCorpus`. Fix #1869 #1911

sj29-innovate · 2018-02-16T09:47:16Z

No description provided.

…into develop

…into develop Conflicts: gensim/summarization/bm25.py

menshikh-iv

Oh, looks really scary at all, much complicated.
Is it possible to hide most of this stuff (maybe using @context_manager)?

menshikh-iv · 2018-02-16T12:02:18Z

gensim/matutils.py

-        with utils.file_or_filename(self.input) as lines:
+
+        # 'with' statement behaviour without closing the file object
+        mgr = (utils.file_or_filename(self.input))


useless ()

menshikh-iv · 2018-02-16T12:02:48Z

gensim/matutils.py

+
+        # 'with' statement behaviour without closing the file object
+        mgr = (utils.file_or_filename(self.input))
+        exit = type(mgr).__exit__


exit is built-in function, don't re-define it

menshikh-iv · 2018-02-16T12:03:35Z

gensim/matutils.py

+                        if not self.transposed:
+                            self.num_docs, self.num_terms = self.num_terms, self.num_docs
+                        break
+            except RuntimeError:


why RuntimeError ?

menshikh-iv · 2018-02-16T12:04:33Z

gensim/matutils.py

+        mgr = (utils.file_or_filename(self.input))
+        exit = type(mgr).__exit__
+        value = type(mgr).__enter__(mgr)
+        exc = True


Shouldn't be inverted (i.e. false by default and True if exception)?

menshikh-iv

Now, this looks significantly more readable, good work.

menshikh-iv · 2018-02-19T06:29:18Z

gensim/matutils.py

@@ -1328,10 +1328,23 @@ class MmReader(object):
    This allows us to process corpora which are larger than the available RAM.

    """
+    @contextmanager


Please move this function to utils.py (this isn't related with MmReader, this can be useful in other cases)

menshikh-iv · 2018-02-19T06:29:27Z

gensim/matutils.py

@@ -1328,10 +1328,23 @@ class MmReader(object):
    This allows us to process corpora which are larger than the available RAM.

    """
+    @contextmanager
+    def open_file(self, input):


docstring in numpy-style please

menshikh-iv · 2018-02-19T06:31:48Z

gensim/matutils.py

+        exc = False
+        try:
+            yield mgr
+        except StandardError:


please add the comment, what's a reason for exception here

Need an exception here to handle any unhandled exceptions in the code nested in the with statement.

if handle any - why not Execption instead of StandartError ?

Hmm , ya my bad . I will use except Exception.

menshikh-iv · 2018-02-19T06:41:57Z

gensim/matutils.py

+            yield mgr
+        except StandardError:
+            exc = True
+            if not exit(mgr, *sys.exc_info()):


I'm slightly worried about exit, how it works?

In case of the exception , 'with' statement has the ability to silence errors. Here trying to close the file with the exception information if possible , otherwise raise the exception.

Is this call __exit__ ? I'm worried because we have exit() function in python

Ya , it is exit() , I will correct it as in the finally block. Thanks.

menshikh-iv · 2018-02-19T06:42:52Z

gensim/test/test_corpora.py

@@ -225,6 +225,14 @@ def test_serialize_compressed(self):
        # MmCorpus needs file write with seek => doesn't support compressed output (only input)
        pass

+    def test_closed_file_object(self):


need to add test for your context manager too (with file path (existent, non-existent), file-like object)

Is it fine if I test whether the context manager throws exception or not for various cases ?

I think yes

menshikh-iv · 2018-02-19T11:18:52Z

gensim/test/test_utils.py

@@ -84,6 +85,48 @@ def test_decode_entities(self):
        expected = u'It\x92s the Year of the Horse. YES VIN DIESEL \U0001f64c \U0001f4af'
        self.assertEqual(utils.decode_htmlentities(body), expected)

+    def test_open_file_existent_file(self):


No need to check that this doesn't raise an exception if something goes wrong - unittest fail the current test and will show needed information (here and everywhere). Sorry, apparently I misunderstood you in the previous question

menshikh-iv · 2018-02-19T11:19:28Z

gensim/test/test_utils.py

+
+        self.assertFalse(exc)
+
+    def test_open_file_non_existent_file(self):


Please use something like Please use something like https://docs.python.org/2/library/unittest.html#unittest.TestCase.assertRaises (instead of manual wrapping to try/exc )

menshikh-iv · 2018-02-19T11:21:07Z

gensim/utils.py

+    except Exception:
+        # Handling any unhandled exceptions from the code nested in 'with' statement.
+        exc = True
+        if not type(mgr).__exit__(mgr, *sys.exc_info()):


why type is needed here?

As per the so thread , " exit = type(mgr).exit is used because CPython has a history of caching special methods and this just makes it consistent ". I tried to implement based on that. mgr.exit() should also work just fine.

menshikh-iv · 2018-02-19T11:22:25Z

gensim/test/test_utils.py

+        exc = False
+        try:
+            with utils.open_file(datapath('non_existent_file.txt')):
+                pass


Please iterate through this file too (not enough only open it), here and everywhere.
Simplest way looks like like

with utils.open_file("...") as infile: self.assertEqual(sum(1 for _ in infile), numer_of_lines_in_file)

menshikh-iv · 2018-02-20T05:00:57Z

gensim/utils.py

+            raise
+        # Try to introspect and silence errors.
+    finally:
+        if not exc and isinstance(input, string_types):


if exc=True and input is file-obj, what happens?

Hmm. I think I missed checking for string type there. Will do that.

menshikh-iv · 2018-02-20T05:02:50Z

gensim/utils.py

+    except Exception:
+        # Handling any unhandled exceptions from the code nested in 'with' statement.
+        exc = True
+        if not mgr.__exit__(*sys.exc_info()):


what return __exit__ ?

Sorry , I didn't understand your query.

X = mgr.__exit__(*sys.exc_info()) , what is X in this case?

x is True if the exception was handled gracefully by " exit() " , else False. In case of file object should we be raising the exception if it occurs ? because we cannot exit it.

menshikh-iv · 2018-02-21T15:06:24Z

Ping @sj29-innovate, can you resolve merge-conflict please

menshikh-iv · 2018-02-22T00:28:29Z

@sj29-innovate you broken something in your code, tests didn't passed correctly, please have a look to Travis logs

sj29-innovate · 2018-02-22T11:41:35Z

@menshikh-iv The test is running fine locally , can it be the cython support for in _mmreader that caused the test to break ?

menshikh-iv · 2018-02-22T18:42:17Z

@sj29-innovate what you did in the last commit? Re-build _mmcorpus.c with Cython?

Hm, I see new signatures, but not see updates in pyx file

sj29-innovate · 2018-02-22T18:43:51Z

@menshikh-iv Yes i rebuild it using cython , there is an update in .pyx , replace "utils.file_or_filename" with "utils.open_file"

sj29-innovate · 2018-02-22T20:17:58Z

gensim/corpora/_mmreader.pyx

@@ -65,7 +65,7 @@ cdef class MmReader(object):
        """
        logger.info("initializing cython corpus reader from %s", input)
        self.input, self.transposed = input, transposed
-        with utils.file_or_filename(self.input) as lines:
+        with utils.open_file(self.input) as lines:


@menshikh-iv Here is the change

@sj29-innovate yes, I see, thanks

menshikh-iv · 2018-02-22T23:09:27Z

Need to merge #1910 first, sorry for waiting @sj29-innovate

sj29-innovate · 2018-02-23T04:08:37Z

@menshikh-iv Can you please suggest some other task ? . Thanks.

menshikh-iv · 2018-02-23T04:10:31Z

@sj29-innovate help guys with #1901 (you can try to setup MacOSX build in distinct PR yourself)

# Conflicts: # gensim/corpora/_mmreader.c

menshikh-iv · 2018-02-23T06:35:16Z

Thank you @sj29-innovate 👍

sj29-innovate added 3 commits January 8, 2018 13:26

bm25 scoring function updated

f9803cb

Merge branch 'develop' of https://github.com/RaRe-Technologies/gensim …

ef8e918

…into develop

Merge branch 'develop' of https://github.com/RaRe-Technologies/gensim …

33a88f4

…into develop Conflicts: gensim/summarization/bm25.py

menshikh-iv suggested changes Feb 16, 2018

View reviewed changes

menshikh-iv suggested changes Feb 19, 2018

View reviewed changes

menshikh-iv suggested changes Feb 20, 2018

View reviewed changes

sj29-innovate force-pushed the new-develop branch 2 times, most recently from 584f66d to 655fe39 Compare February 21, 2018 21:25

Fixes 1869 , Fresh commits for all changes

b5db34c

sj29-innovate force-pushed the new-develop branch from 655fe39 to b5db34c Compare February 21, 2018 21:46

Fixes 1869 , Fresh commits for all changes

644f97a

sj29-innovate force-pushed the new-develop branch from f8c26d2 to 644f97a Compare February 21, 2018 22:07

Fixes 1869 , Fresh commits for all changes

6cdc488

sj29-innovate force-pushed the new-develop branch from 4b44735 to 6cdc488 Compare February 21, 2018 23:00

sj29-innovate added 3 commits February 22, 2018 17:18

Fixes 1869 , Fresh commits for all changes

2d75847

Fixes 1869 , Fresh commits for all changes

1463987

Fixes 1869 , Fresh commits for all changes

c06642d

sj29-innovate commented Feb 22, 2018

View reviewed changes

menshikh-iv added 3 commits February 23, 2018 09:38

Merge remote-tracking branch 'upstream/develop' into new-develop

70aa324

# Conflicts: # gensim/corpora/_mmreader.c

fix reader after conflict

db6f458

fix docstring

73be314

menshikh-iv changed the title ~~Fixes #1869 , Mmcorpus file object behaviour fixed~~ Fix file-like closing bug from gensim.corpora.MmCorpus. Fix #1869 Feb 23, 2018

menshikh-iv merged commit c1e6c65 into piskvorky:develop Feb 23, 2018


		self.assertFalse(exc)

		def test_open_file_non_existent_file(self):

Fix file-like closing bug from gensim.corpora.MmCorpus. Fix #1869 #1911

Fix file-like closing bug from gensim.corpora.MmCorpus. Fix #1869 #1911

Conversation

sj29-innovate commented Feb 16, 2018

menshikh-iv 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

menshikh-iv 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

menshikh-iv Feb 19, 2018 • edited Loading

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

sj29-innovate Feb 19, 2018 • edited Loading

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

sj29-innovate Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

menshikh-iv commented Feb 21, 2018

menshikh-iv commented Feb 22, 2018

sj29-innovate commented Feb 22, 2018

menshikh-iv commented Feb 22, 2018 • edited Loading

sj29-innovate commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menshikh-iv commented Feb 22, 2018

sj29-innovate commented Feb 23, 2018

menshikh-iv commented Feb 23, 2018

menshikh-iv commented Feb 23, 2018

Fix file-like closing bug from `gensim.corpora.MmCorpus`. Fix #1869 #1911

Fix file-like closing bug from `gensim.corpora.MmCorpus`. Fix #1869 #1911

menshikh-iv Feb 19, 2018 •

edited

Loading

sj29-innovate Feb 19, 2018 •

edited

Loading

sj29-innovate Feb 20, 2018 •

edited

Loading

menshikh-iv commented Feb 22, 2018 •

edited

Loading

sj29-innovate commented Feb 22, 2018 •

edited

Loading