search.Beam: Fix invalid indexing

[danieldk]

If none of the states of the beam have valid moves, it was attempted to index into the vector of scored valid moves anyway, which results in invalid memory access.

Fix this by first checking that there valid moves before attempting to index into them.

@honnibal: I am not sure if this is the valid way to address this issue, but this is the source of access violations on Windows in explosion/spaCy#10633.

[adrianeboyd]

It is weird that the CI didn't run, I'll close and reopen to see what happens.

[svlandeg]

Looks good to me. If Matt agrees, we should probably do a release of Thinc to get the bug fixed in spaCy?

[honnibal]

This is a fine guard but it should be impossible to have no valid continuations to a state. So is there a deeper problem here?

[danieldk]

This is a fine guard but it should be impossible to have no valid continuations to a state. So is there a deeper problem here?

It is caused by this test:

https:/explosion/spaCy/blob/29afbdb91e5fecf513125a85f1ac1d165f40bc93/spacy/tests/parser/test_ner.py#L183

I guess there are no continuations because the pipe is not trained, so all labels that were added are in unseen_classes and can not be predicted?

[svlandeg]

So should we somehow try to catch this earlier and provide a more meaningful user warning/error?

[danieldk]

So should we somehow try to catch this earlier and provide a more meaningful user warning/error?

Let me try and see if I can add a warning to spaCy. Edit: I guess spaCy is the best place, because in Thinc this is a utility class and there may be applications where this is an acceptable outcome?

There still seems to be a deeper issue, since this doesn't happen in the parser in spaCy master.

[danieldk]

Found the issue: the Parser.beam_parse/greedy_parse methods in my branch didn't ensure that labels were added. I have fixed this in the PR that adds back beam parsing (explosion/spaCy#10633). It should now (hopefully) also pass on Windows without this PR.

[svlandeg]

Great! We should still merge this as well though :-)