improve error messages for invalid indexing configurations

[cmacdonald]

This addresses the experience reported in #348 by @maxhenze

[seanmacavaney]

Along this line, a few situations I've seen students encounter recently that could use better indexing error messages:

• Short circuit error message if a docno is encountered that's longer than the configured maximum. (Currently no error thrown, only know once you get very poor performance when retrieving).
• Clearer error message when user passes non-dict objects to IterDictIndexer (it's caught in Java and is rather cryptic)

[cmacdonald]

Clearer error message when user passes non-dict objects to IterDictIndexer (it's caught in Java and is rather cryptic)

I have introduced a check for this on the first document. There is a related issue (tracking at terrier-org/terrier-core#215) of not propagating the Java exception back into Python by FLATJsonDocument that we can fix upstream for a future release.

Short circuit error message if a docno is encountered that's longer than the configured maximum. (Currently no error thrown, only know once you get very poor performance when retrieving).

Actually, this is caused by 'metaindex.compressed.crop.long' : 'true' which you have for IterDictIndexer (presumably to make text cropping automatic) - see https://github.com/terrier-org/pyterrier/blob/master/pyterrier/index.py#L803. We could pass the meta lengths to FlatJSONDocumentIterator and check the validity of the first document? This wont handle all possible error modes (e.g. first document is fine, but latter document is not), but would help, right?

[seanmacavaney]

Awesome, thanks!

This wont handle all possible error modes (e.g. first document is fine, but latter document is not), but would help, right?

Yeah, this would probably cover most cases and would be a huge help.

But cases where things are numbered (e.g., %p1 ... %p10) could consistently fail. The error message in the passage case would be that a duplicate docno was found, and the root cause could be tricky to get to the bottom of without this check.

The reasoning for only checking the first one is that checking every docno would get expensive? I imagine that it wouldn't add all that much overhead, especially as compared to the JSON generation, crossing between Python/Java, doing the actual indexing, etc.

[cmacdonald]

Ok, I have adjusted FlatJSONDocumentIterator to handle first doc. I note that FlatJSONDocumentIterator is only used by the non-FIFO setting, ie Windows, so these improvements would not work on Linux/macOS. The fifo impl seems to do more things natively in Java, and I cant see easily how to fix this.

Aside: the fifo impl is more complicated and possibly unnecessary if the default is threads=1.

[seanmacavaney]

and I cant see easily how to fix this

Should be able to just peek at the first doc here, no? If you like, I could maybe take a look.

Aside: the fifo impl is more complicated and possibly unnecessary if the default is threads=1.

I suspect it's far more efficient though. From what I've seen benchmarking jnius, it's costly to move between Python and Java (which the nofifo one needs to do for every document, while the fifo one does not).

[cmacdonald]

If you like, I could maybe take a look.

Sure, go for it.

[seanmacavaney]

I'm not sure why self._filter_iterable wasn't here before, as it is in the other invocation below.

[cmacdonald]

dont think I understood its purpose....

[seanmacavaney]

@cmacdonald -- updated! Mind taking a look when you have a chance?

I also changed the warning to an error in the case of docno. I feel that if your docnos are being truncated, it is always a problem and it's best to force the user to deal with it before indexing will start. Other fields do not matter as much, so they remain just a warning.

[cmacdonald]

lgtm, after discussion