Repository: explosion/spaCy
Pull request: Fix listener architecture with empty doc in batch
Fixes #7029 

## Description
The listener returns some random data with a valid shape for shape inference, when `doc.tensor` isn't set. However using `all` for this check, means that it will hit also if we have an empty document in the batch - effectively wiping the previously calculated `tok2vec` results for the non-empty documents in the same batch. We could use `any` instead as a minimal fix, but really I think we might need to revise this code bit altogether - it's really bug-prone :(

### Types of change
bug fix

## Checklist
- [x] I have submitted the spaCy Contributor Agreement.
- [x] I ran the tests, and all new and existing tests passed.
- [x] My changes don't require a change to the documentation, or if they do, I've added all required information.


Problem statement:
POS will be all "ADP" when the inputs have empty string when using .pipe()
## How to reproduce the behaviour
When I use `spacy_model.pipe()` to process documents in parallel, the POS tagging will give wrong result if there is any empty string int the documents.
For example:
(1) Normally:
```
docs=["Hello world!","How are you?"]
spacy_docs=spacy_model.pipe(docs)
for spacy_doc in spacy_docs:
    for sentence in spacy_doc.sents:
        print([(token.text,token.pos_) for token in sentence])
```
Output:
```
[('Hello', 'INTJ'), ('world', 'NOUN'), ('!', 'PUNCT')]
[('How', 'ADV'), ('are', 'VERB'), ('you', 'PRON'), ('?', 'PUNCT')]
```
(2) Input has empty string:
```
docs=["Hello world!","How are you?",""]
spacy_docs=spacy_model.pipe(docs)
for spacy_doc in spacy_docs:
    for sentence in spacy_doc.sents:
        print([(token.text,token.pos_) for token in sentence])
```
Output:
```
[('Hello', 'ADP'), ('world', 'ADP'), ('!', 'ADP')]
[('How', 'ADP'), ('are', 'ADP'), ('you', 'ADP'), ('?', 'ADP')]
```
In my opinion, the empty string should not affect the output of other documents. I think there is bug.

## Your Environment
### Info about spaCy
- **spaCy version:** 3.0.1
- **Platform:** Darwin-18.7.0-x86_64-i386-64bit
- **Python version:** 3.6.10
- **Pipelines:** en_core_web_sm (3.0.0)

* Operating System: MacOS and Linux
* Python Version Used: python 3.6 and python 3.7
* spaCy Version Used: 3.0.1
* Environment Information:

Hints from issue discussion:
Thanks for the report! I can reproduce this and it definitely looks like a bug. We will look into it!
Hi @adrianeboyd , anyone working on this? I would like to contribute.
Hi @PradipNichite ,

If you want to have a look at this, please do go ahead! It's possible Adriane is looking into it as well but it's almost the end of the work day in Germany. In any case, if you develop tests for the bug, they're unlikely to be the same as the ones someone else develops, so the effort probably won't be wasted.

I think the bug will almost certainly be within the `spacy-transformers` package, likely to do with the alignment. You might be able to see something wrong if you look at the `doc._.trf_data.wordpieces` object, especially the `doc._.trf_data.wordpieces.align`. Here are some source links that might be helpful:

* The `WordpieceBatch` data structure: https://github.com/explosion/spacy-transformers/blob/master/spacy_transformers/data_classes.py#L16
* The `TransformerData` data structure: https://github.com/explosion/spacy-transformers/blob/master/spacy_transformers/data_classes.py#L131
* The function where the interaction with the transformer happens: https://github.com/explosion/spacy-transformers/blob/master/spacy_transformers/layers/transformer_model.py#L116
* The place where the transformer data is set onto the `Doc` object: https://github.com/explosion/spacy-transformers/blob/master/spacy_transformers/pipeline_component.py#L224
Hi @honnibal , Thanks for the help. I am new to open source contribution and first time with Spacy.  I might take more time.
I will try to understand the bug. if @adrianeboyd  already looking into it then fine. I will look for another issue, please let me know if you find any Good first Issue.

No, this is really bad and happens with all models. The other docs in the same batch as the empty doc have messed up predictions across all components:

```python
import spacy
nlp = spacy.load("en_core_web_sm")

texts = ["a", "a", "a", "a", "a", "a", "a", ""]
docs1 = list(nlp.pipe(texts, batch_size=1))
docs2 = list(nlp.pipe(texts, batch_size=4))
print([doc[0].tag_ for doc in docs1[:-1]])
print([doc[0].tag_ for doc in docs2[:-1]])
```

```none
['DT', 'DT', 'DT', 'DT', 'DT', 'DT', 'DT']
['DT', 'DT', 'DT', 'DT', 'IN', 'IN', 'IN']
```

I haven't figured it out yet, but I'm slow at debugging thinc internals.
It's something in the tok2vec, so actually not at all with the transformers models.
The input to the softmax layer is 0s for the whole batch.

Commit message: remove ner

Patch to review:
diff --git a/spacy/pipeline/tok2vec.py b/spacy/pipeline/tok2vec.py
index 61ba498c9f4..4a396eaeb26 100644
--- a/spacy/pipeline/tok2vec.py
+++ b/spacy/pipeline/tok2vec.py
@@ -291,7 +291,7 @@ def forward(model: Tok2VecListener, inputs, is_train: bool):
         # of data.
         # When the components batch differently, we don't receive a matching
         # prediction from the upstream, so we can't predict.
-        if not all(doc.tensor.size for doc in inputs):
+        if not any(doc.tensor.size for doc in inputs):
             # But we do need to do *something* if the tensor hasn't been set.
             # The compromise is to at least return data of the right shape,
             # so the output is valid.
diff --git a/spacy/tests/regression/test_issue7029.py b/spacy/tests/regression/test_issue7029.py
new file mode 100644
index 00000000000..2ff730e2968
--- /dev/null
+++ b/spacy/tests/regression/test_issue7029.py
@@ -0,0 +1,68 @@
+from spacy.lang.en import English
+from spacy.training import Example
+from spacy.util import load_config_from_str
+
+
+CONFIG = """
+[nlp]
+lang = "en"
+pipeline = ["tok2vec", "tagger"]
+
+[components]
+
+[components.tok2vec]
+factory = "tok2vec"
+
+[components.tok2vec.model]
+@architectures = "spacy.Tok2Vec.v1"
+
+[components.tok2vec.model.embed]
+@architectures = "spacy.MultiHashEmbed.v1"
+width = ${components.tok2vec.model.encode:width}
+attrs = ["NORM","PREFIX","SUFFIX","SHAPE"]
+rows = [5000,2500,2500,2500]
+include_static_vectors = false
+
+[components.tok2vec.model.encode]
+@architectures = "spacy.MaxoutWindowEncoder.v1"
+width = 96
+depth = 4
+window_size = 1
+maxout_pieces = 3
+
+[components.tagger]
+factory = "tagger"
+
+[components.tagger.model]
+@architectures = "spacy.Tagger.v1"
+nO = null
+
+[components.tagger.model.tok2vec]
+@architectures = "spacy.Tok2VecListener.v1"
+width = ${components.tok2vec.model.encode:width}
+upstream = "*"
+"""
+
+
+TRAIN_DATA = [
+    ("I like green eggs", {"tags": ["N", "V", "J", "N"]}),
+    ("Eat blue ham", {"tags": ["V", "J", "N"]}),
+]
+
+
+def test_issue7029():
+    """Test that an empty document doesn't mess up an entire batch.
+    """
+    nlp = English.from_config(load_config_from_str(CONFIG))
+    train_examples = []
+    for t in TRAIN_DATA:
+        train_examples.append(Example.from_dict(nlp.make_doc(t[0]), t[1]))
+    optimizer = nlp.initialize(get_examples=lambda: train_examples)
+    for i in range(50):
+        losses = {}
+        nlp.update(train_examples, sgd=optimizer, losses=losses)
+    texts = ["first", "second", "thrid", "fourth", "and", "then", "some", ""]
+    nlp.select_pipes(enable=["tok2vec", "tagger"])
+    docs1 = list(nlp.pipe(texts, batch_size=1))
+    docs2 = list(nlp.pipe(texts, batch_size=4))
+    assert [doc[0].tag_ for doc in docs1[:-1]] == [doc[0].tag_ for doc in docs2[:-1]]


Code review comments:
diff_hunk: @@ -291,7 +291,7 @@ def forward(model: Tok2VecListener, inputs, is_train: bool):
         # of data.
         # When the components batch differently, we don't receive a matching
         # prediction from the upstream, so we can't predict.
-        if not all(doc.tensor.size for doc in inputs):
+        if not any(doc.tensor.size for doc in inputs):
path: spacy/pipeline/tok2vec.py
text: @user1:
Thanks for finding this!

I can't decide whether it's more or less brittle to do it as a loop?

```
outputs = []
width = model.get_dim("nO")
for doc in docs:
    if doc.tensor.size == 0:
        outputs.append(model.ops.alloc2f(len(doc), width))
    else:
        outputs.append(doc.tensor)
```

Maybe the loop is easier to understand at least?

@author:
I'm definitely fine with the loop, but I still think we should consider warning here. In most cases, this code path results in trouble. I know we need it for initialization & shape inference, but we could have a clever way (hopefully) to avoid the warnings in that specific case, no?

@user1:
I see what you're saying but I don't want to add more guessing right now. Let's get the loop in and get this fix deployed, and then plot our next mitigation. 😬