search

Georgetown-IR-Lab / OpenNIR

An end-to-end neural ad-hoc ranking pipeline.
https://opennir.net
MIT License
150 stars 25 forks source link

Fixes issue with StopIteration exceptions using python 3.7 #32

Closed cash closed 3 years ago

cash commented 3 years ago

Python 3.7 changes how StopIteration exceptions internal to a generator are handled. Before they were silently swallowed and now they are converted to runtime exceptions. This causes issues with blocking tee code in OpenNIR. BlockingTeeControllerThread's run method catches the StopIteration and and stores it. _blocking_tee_iter() access that exception and re-raises it. This worked in earlier Python versions, but the preferred manner to handle this is to catch internal StopIterations and terminate the generator.

I added a test for exception type so that a file not found exception or similar would still bubble up the stack.

I'll also mention that this code isn't doing what is expected:

        if value is not StopIteration:
            yield value

The function next() raises a StopIteration exception so ctrl.value is not updated. (The older style iter.next() would return a StopIteration I believe). If there was a reason to test for StopIteration, it should be done with isinstance. It is not hurting anything because the is not test is always true, but should probably be changed to simple yield the value without the test. I can do that on this PR if desired.

seanmacavaney commented 3 years ago

Thanks! Looks good to me. I believe this is a fix for the issue brought up in #31.

I suspect the implementation was originally trying to use StopIteration as a sentinel value there, much like it does here: https://github.com/Georgetown-IR-Lab/OpenNIR/blob/onir/interfaces/trec.py#L269. But you're right that it's not doing anything here, so it can be removed.