Repository: explosion/spaCy
Pull request: Span/SpanGroup: wrap SpanC in shared_ptr
<!--- Provide a general summary of your changes in the title. -->

## Description
<!--- Use this section to describe your changes. If your changes required
testing, include information about the testing environment and the tests you
ran. If your test fixes a bug reported in an issue, don't forget to include the
issue number. If your PR is still a work in progress, that's totally fine – just
include a note to let us know. -->

When a `Span` that was retrieved from a `SpanGroup` was modified, these changes were not reflected in the `SpanGroup` because the underlying `SpanC` struct was copied.

This change applies the solution proposed by @nrodnova, to wrap `SpanC` in a `shared_ptr`. This makes a `SpanGroup` and Spans derived from it share the same `SpanC`. So, changes made through a `Span` are visible in the `SpanGroup` as well.

### Types of change
<!-- What type of change does your PR cover? Is it a bug fix, an enhancement
or new feature, or a change to the documentation? -->

Enhancement

## Checklist
<!--- Before you submit the PR, go over this checklist and make sure you can
tick off all the boxes. [] -> [x] -->
- [x] I confirm that I have the right to submit this contribution under the project's MIT license.
- [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:
SpanGroup doesn't allow access to original spans
## How to reproduce the behaviour
<!-- Include a code example or the steps that led to the problem. Please try to be as specific as possible. -->
Use case: I use a custom entity ruler to populate `doc.spans`, a `SpanGroup` per entity label,  to allow overlapping spans between different entity types. After the NER step is completed, I check for certain conditions and for some spans I change `kb_id`. Looks like `SpanGroup` class creates copies of `Spans` when iterating through them. So the assigned `kb_id` gets lost. There is no elegant solution to the problem except for iterating through each `Span` in a `SpanGroup` and create a new group each time `kb_id` needs to be re-assigned. There is no way to delete or replace a particular span from `SpanGroup`.

Here is code example:

### Set up
```
import spacy
from spacy.tokens import Span, SpanGroup

nlp = spacy.load("en_core_web_sm")

text = "Span 1, Span 2"
doc = nlp(text)

nlp('kb_id_2') # add to the vocab
span_1 = Span(doc, 0, 2, label = "TEST", kb_id = 'kb_id_1')

```
### Iterating through `SpanGroup` and re-assigning `kb_id`
```
doc.spans['TEST'] = [span_1]
for span in doc.spans['TEST'] :
    print("Before: %s" % span.kb_id_)
    span.kb_id = nlp.vocab.strings['kb_id_2']
    print("After: %s" % span.kb_id_)

```
### Output:
```
Before: kb_id_1
After: kb_id_2
```
### Now, iterate through the `SpanGroup` again:
```
for span in doc.spans['TEST'] :
    print(span, "=>", span.kb_id_)
```
### Output (unchanged `kb_id`):
```
Span 1 => kb_id_1
```
Any help will be greatly appreciated.

## Your Environment
<!-- Include details of your environment. If you're using spaCy 1.7+, you can also type `python -m spacy info --markdown` and copy-paste the result here.-->

- **spaCy version:** 3.0.6
- **Platform:** Darwin-20.3.0-x86_64-i386-64bit
- **Python version:** 3.7.9
- **Pipelines:** en_core_web_lg (3.0.0), en_core_web_md (3.0.0), en_core_web_sm (3.0.0)

Hints from issue discussion:
Thanks for the detailed explanation of your use case. You are correct that this is the way the SpanGroup works at the moment. In particular, it's correct the easiest way to change values is to recreate the SpanGroup.

I need to check to be sure but I think it works this way for efficiency or serialization related reasons and this is a side effect of that, so not a bug. However we should definitely make this more apparent in the docs, so thanks for bringing it to our attention.
> I need to check to be sure but I think it works this way for efficiency or serialization related reasons and this is a side effect of that, so not a bug. However we should definitely make this more apparent in the docs, so thanks for bringing it to our attention.

On a related note, the same happens for [Doc.ents](https://spacy.io/api/doc#ents). That's tripped me up before where I only modify the returned Spans and forget to set the entities back on the Doc to apply the changes.
@kinghuang I was able to modify entities directly in the past, haven't checked it recently. But I imagine, something would get broken in my code if it changed. I was able to set `ent_type_` and `ent_id_`, on the token level. But with spans that are in `SpanGroup` it is different, `kb_id_` is not shared with underlying `Tokens` (which I think is right, it allows to have different `kb_ids` for overlapping spans.)
@polm I experimented a bit more, I can change user-assigned attributes in Spans that are a part of SpanGroup. But not `kb_id`, and I imagine, not `label`.
@polm I poked around the code a bit more. And looks like I'll need some advice. First of all, @kinghuang, the reason why I was able to set labels/entity ids without resetting `doc.ents` was that I would actually set values on the underlying `Tokens`. This wouldn't work on potentially overlapping spans in `SpanGroups`.
So, I see what is happening in `SpanGroup`. It doesn't actually store `Spans`, it stores a list of data structures describing `Spans`: `start`, `end`, `label`, `kb_id` and a few other attributes. `SpanGroup.__get_item__` creates a new instance of `Span` using that data structure.  So, whatever values are set, they get discarded unless `SpanGroup` is re-initialized with a new list of `Spans`. I don't like the way it works, but I can work around it. 

However, I discovered another potential problem. I have't looked in `Underscore` class before and didn't realize that `_` values were actually stored in `Doc.user_data` dictionary. The key into the dictionary for `spans` is a tuple `('_', variable_name, start_char, end_char)` which is a problem for overlapping spans. Say, I have 2 `Spans` pointing to the same sequence of tokens, i.e. their `start_char` and `end_char` are identical. These 2 spans can have different labels and kb_ids, but their `_` values would override each other because the key in `Doc.used_data` is based on `start_char` and `end_char`. 

At first, to overcome the problem of not being able to change `kb_id_` on a `Span` within `SpanGroup`, I was thinking of using a custom attribute for `kb_id`. And because of the issue described above, it won't work on the spans pointing to the same tokens. Should I try to override `Underscore` class and add more keys in `Doc.user_data` or you guys have better suggestions?
@polm Sorry for lengthy messages. Do you think it would make sense in `SpanGroup.__getitem__` to instantiate a `Span` that points to `Span.c` structure from `SpanGroup.c` vector? This way, if data members such as label or kb_id are updated, they would get updated on the underlying structure?
@polm I thought a bit more about this. Pointing `Span.c` to an element in the `SpanGroup.c` vector would be a memory management nightmare. Would it be possible at least to add a `__setitem__` function that takes an index and a `Span` and sets the underlying structure? I really hate re-creating `SpanGroup` every time I want to change something :) It would be something like this:
```
def __setitem__(self, i, span)
    cdef int size = self.c.size()
    if i < -size or i >= size:
        raise IndexError(f"list index {i} out of range")
    if i < 0:
        i += size
    if span :
        self.c[i] = span.c # Assuming the copy constructor will be called, sorry I am unfamiliar with Cython

```
I don't see any downside in having this method
Memory management and dealing with the `doc` references in the `Span` objects (vs. `SpanC` structs) is indeed the problem. We have discussed this internally without coming up with a good general solution thus far. We used to make all spans read-only, but that also has a lot of drawbacks. We can have a look at the setter idea, but it's still going to be confusing for people who just try to modify the span directly.
@adrianeboyd Any reason you store `SpanGroup`'s spans in an stl vector? Besides serialization? I am almost tempted to just use a custom dictionary of lists of spans instead of `doc.spans`. This will require some additional work when serializing/de-serializing, but I'd say span manipulation easiness is more important than some extra work during serialization.
The problem is circular references and garbage collection. If you store a list of `Span` objects that include `span.doc` within `Doc.spans`, then the doc never gets garbage collected.

Commit message: Style fix: pointer types

Patch to review:
diff --git a/spacy/pipeline/_parser_internals/ner.pyx b/spacy/pipeline/_parser_internals/ner.pyx
index 3edeff19a01..87410de0f19 100644
--- a/spacy/pipeline/_parser_internals/ner.pyx
+++ b/spacy/pipeline/_parser_internals/ner.pyx
@@ -1,6 +1,8 @@
 import os
 import random
 from libc.stdint cimport int32_t
+from libcpp.memory cimport shared_ptr
+from libcpp.vector cimport vector
 from cymem.cymem cimport Pool
 
 from collections import Counter
@@ -42,9 +44,7 @@ MOVE_NAMES[OUT] = 'O'
 
 cdef struct GoldNERStateC:
     Transition* ner
-    SpanC* negs
-    int32_t length
-    int32_t nr_neg
+    vector[shared_ptr[SpanC]] negs
 
 
 cdef class BiluoGold:
@@ -77,8 +77,6 @@ cdef GoldNERStateC create_gold_state(
         negs = []
     assert example.x.length > 0
     gs.ner = <Transition*>mem.alloc(example.x.length, sizeof(Transition))
-    gs.negs = <SpanC*>mem.alloc(len(negs), sizeof(SpanC))
-    gs.nr_neg = len(negs)
     ner_ents, ner_tags = example.get_aligned_ents_and_ner()
     for i, ner_tag in enumerate(ner_tags):
         gs.ner[i] = moves.lookup_transition(ner_tag)
@@ -92,8 +90,8 @@ cdef GoldNERStateC create_gold_state(
     # In order to handle negative samples, we need to maintain the full
     # (start, end, label) triple. If we break it down to the 'isnt B-LOC'
     # thing, we'll get blocked if there's an incorrect prefix.
-    for i, neg in enumerate(negs):
-        gs.negs[i] = neg.c
+    for neg in negs:
+        gs.negs.push_back(neg.c)
     return gs
 
 
@@ -410,6 +408,8 @@ cdef class Begin:
         cdef int g_act = gold.ner[b0].move
         cdef attr_t g_tag = gold.ner[b0].label
 
+        cdef shared_ptr[SpanC] span
+
         if g_act == MISSING:
             pass
         elif g_act == BEGIN:
@@ -427,8 +427,8 @@ cdef class Begin:
             # be correct or not. However, we can at least tell whether we're
             # going to be opening an entity where there's only one possible
             # L.
-            for span in gold.negs[:gold.nr_neg]:
-                if span.label == label and span.start == b0:
+            for span in gold.negs:
+                if span.get().label == label and span.get().start == b0:
                     cost += 1
                     break
         return cost
@@ -573,8 +573,9 @@ cdef class Last:
         # If we have negative-example entities, integrate them into the objective,
         # by marking actions that close an entity that we know is incorrect
         # as costly.
-        for span in gold.negs[:gold.nr_neg]:
-            if span.label == label and (span.end-1) == b0 and span.start == ent_start:
+        cdef shared_ptr[SpanC] span
+        for span in gold.negs:
+            if span.get().label == label and (span.get().end-1) == b0 and span.get().start == ent_start:
                 cost += 1
                 break
         return cost
@@ -638,8 +639,9 @@ cdef class Unit:
         # This is fairly straight-forward for U- entities, as we have a single
         # action
         cdef int b0 = s.B(0)
-        for span in gold.negs[:gold.nr_neg]:
-            if span.label == label and span.start == b0 and span.end == (b0+1):
+        cdef shared_ptr[SpanC] span
+        for span in gold.negs:
+            if span.get().label == label and span.get().start == b0 and span.get().end == (b0+1):
                 cost += 1
                 break
         return cost
diff --git a/spacy/tests/doc/test_span.py b/spacy/tests/doc/test_span.py
index 10aba5b9434..1752df486a7 100644
--- a/spacy/tests/doc/test_span.py
+++ b/spacy/tests/doc/test_span.py
@@ -4,7 +4,7 @@
 
 from spacy.attrs import ORTH, LENGTH
 from spacy.lang.en import English
-from spacy.tokens import Doc, Span, Token
+from spacy.tokens import Doc, Span, SpanGroup, Token
 from spacy.vocab import Vocab
 from spacy.util import filter_spans
 from thinc.api import get_current_ops
@@ -163,6 +163,17 @@ def test_char_span(doc, i_sent, i, j, text):
         assert span.text == text
 
 
+def test_modify_span_group(doc):
+    group = SpanGroup(doc, spans=doc.ents)
+    for span in group:
+        span.start = 0
+        span.label = doc.vocab.strings["TEST"]
+
+    # Span changes must be reflected in the span group
+    assert group[0].start == 0
+    assert group[0].label == doc.vocab.strings["TEST"]
+
+
 def test_spans_sent_spans(doc):
     sents = list(doc.sents)
     assert sents[0].start == 0
diff --git a/spacy/tokens/span.pxd b/spacy/tokens/span.pxd
index 78bee0a8cc3..85553068e65 100644
--- a/spacy/tokens/span.pxd
+++ b/spacy/tokens/span.pxd
@@ -1,3 +1,4 @@
+from libcpp.memory cimport shared_ptr
 cimport numpy as np
 
 from .doc cimport Doc
@@ -7,19 +8,21 @@ from ..structs cimport SpanC
 
 cdef class Span:
     cdef readonly Doc doc
-    cdef SpanC c
+    cdef shared_ptr[SpanC] c
     cdef public _vector
     cdef public _vector_norm
 
     @staticmethod
-    cdef inline Span cinit(Doc doc, SpanC span):
+    cdef inline Span cinit(Doc doc, const shared_ptr[SpanC] &span):
         cdef Span self = Span.__new__(
             Span,
             doc,
-            start=span.start,
-            end=span.end
+            start=span.get().start,
+            end=span.get().end
         )
         self.c = span
         return self
 
     cpdef np.ndarray to_array(self, object features)
+
+    cdef SpanC* span_c(self)
diff --git a/spacy/tokens/span.pyx b/spacy/tokens/span.pyx
index cd02cab3653..c8fb0d1a296 100644
--- a/spacy/tokens/span.pyx
+++ b/spacy/tokens/span.pyx
@@ -1,5 +1,6 @@
 cimport numpy as np
 from libc.math cimport sqrt
+from libcpp.memory cimport make_shared
 
 import numpy
 from thinc.api import get_array_module
@@ -109,14 +110,14 @@ cdef class Span:
             end_char = start_char
         else:
             end_char = doc[end - 1].idx + len(doc[end - 1])
-        self.c = SpanC(
+        self.c = make_shared[SpanC](SpanC(
             label=label,
             kb_id=kb_id,
             start=start,
             end=end,
             start_char=start_char,
             end_char=end_char,
-        )
+        ))
         self._vector = vector
         self._vector_norm = vector_norm
 
@@ -126,41 +127,46 @@ cdef class Span:
                 return False
             else:
                 return True
+
+        cdef SpanC* span_c = self.span_c()
+        cdef SpanC* other_span_c = other.span_c()
+
         # <
         if op == 0:
-            return self.c.start_char < other.c.start_char
+            return span_c.start_char < other_span_c.start_char
         # <=
         elif op == 1:
-            return self.c.start_char <= other.c.start_char
+            return span_c.start_char <= other_span_c.start_char
         # ==
         elif op == 2:
             # Do the cheap comparisons first
             return (
-                (self.c.start_char == other.c.start_char) and \
-                (self.c.end_char == other.c.end_char) and \
-                (self.c.label == other.c.label) and \
-                (self.c.kb_id == other.c.kb_id) and \
+                (span_c.start_char == other_span_c.start_char) and \
+                (span_c.end_char == other_span_c.end_char) and \
+                (span_c.label == other_span_c.label) and \
+                (span_c.kb_id == other_span_c.kb_id) and \
                 (self.doc == other.doc)
             )
         # !=
         elif op == 3:
             # Do the cheap comparisons first
             return not (
-                (self.c.start_char == other.c.start_char) and \
-                (self.c.end_char == other.c.end_char) and \
-                (self.c.label == other.c.label) and \
-                (self.c.kb_id == other.c.kb_id) and \
+                (span_c.start_char == other_span_c.start_char) and \
+                (span_c.end_char == other_span_c.end_char) and \
+                (span_c.label == other_span_c.label) and \
+                (span_c.kb_id == other_span_c.kb_id) and \
                 (self.doc == other.doc)
             )
         # >
         elif op == 4:
-            return self.c.start_char > other.c.start_char
+            return span_c.start_char > other_span_c.start_char
         # >=
         elif op == 5:
-            return self.c.start_char >= other.c.start_char
+            return span_c.start_char >= other_span_c.start_char
 
     def __hash__(self):
-        return hash((self.doc, self.c.start_char, self.c.end_char, self.c.label, self.c.kb_id))
+        cdef SpanC* span_c = self.span_c()
+        return hash((self.doc, span_c.start_char, span_c.end_char, span_c.label, span_c.kb_id))
 
     def __len__(self):
         """Get the number of tokens in the span.
@@ -169,9 +175,10 @@ cdef class Span:
 
         DOCS: https://spacy.io/api/span#len
         """
-        if self.c.end < self.c.start:
+        cdef SpanC* span_c = self.span_c()
+        if span_c.end < span_c.start:
             return 0
-        return self.c.end - self.c.start
+        return span_c.end - span_c.start
 
     def __repr__(self):
         return self.text
@@ -185,15 +192,16 @@ cdef class Span:
 
         DOCS: https://spacy.io/api/span#getitem
         """
+        cdef SpanC* span_c = self.span_c()
         if isinstance(i, slice):
             start, end = normalize_slice(len(self), i.start, i.stop, i.step)
             return Span(self.doc, start + self.start, end + self.start)
         else:
             if i < 0:
-                token_i = self.c.end + i
+                token_i = span_c.end + i
             else:
-                token_i = self.c.start + i
-            if self.c.start <= token_i < self.c.end:
+                token_i = span_c.start + i
+            if span_c.start <= token_i < span_c.end:
                 return self.doc[token_i]
             else:
                 raise IndexError(Errors.E1002)
@@ -205,7 +213,8 @@ cdef class Span:
 
         DOCS: https://spacy.io/api/span#iter
         """
-        for i in range(self.c.start, self.c.end):
+        cdef SpanC* span_c = self.span_c()
+        for i in range(span_c.start, span_c.end):
             yield self.doc[i]
 
     def __reduce__(self):
@@ -213,9 +222,10 @@ cdef class Span:
 
     @property
     def _(self):
+        cdef SpanC* span_c = self.span_c()
         """Custom extension attributes registered via `set_extension`."""
         return Underscore(Underscore.span_extensions, self,
-                          start=self.c.start_char, end=self.c.end_char)
+                          start=span_c.start_char, end=span_c.end_char)
 
     def as_doc(self, *, bint copy_user_data=False, array_head=None, array=None):
         """Create a `Doc` object with a copy of the `Span`'s data.
@@ -289,13 +299,14 @@ cdef class Span:
         cdef int length = len(array)
         cdef attr_t value
         cdef int i, head_col, ancestor_i
+        cdef SpanC* span_c = self.span_c()
         old_to_new_root = dict()
         if HEAD in attrs:
             head_col = attrs.index(HEAD)
             for i in range(length):
                 # if the HEAD refers to a token outside this span, find a more appropriate ancestor
                 token = self[i]
-                ancestor_i = token.head.i - self.c.start   # span offset
+                ancestor_i = token.head.i - span_c.start   # span offset
                 if ancestor_i not in range(length):
                     if DEP in attrs:
                         array[i, attrs.index(DEP)] = dep
@@ -303,7 +314,7 @@ cdef class Span:
                     # try finding an ancestor within this span
                     ancestors = token.ancestors
                     for ancestor in ancestors:
-                        ancestor_i = ancestor.i - self.c.start
+                        ancestor_i = ancestor.i - span_c.start
                         if ancestor_i in range(length):
                             array[i, head_col] = ancestor_i - i
 
@@ -332,7 +343,8 @@ cdef class Span:
 
         DOCS: https://spacy.io/api/span#get_lca_matrix
         """
-        return numpy.asarray(_get_lca_matrix(self.doc, self.c.start, self.c.end))
+        cdef SpanC* span_c = self.span_c()
+        return numpy.asarray(_get_lca_matrix(self.doc, span_c.start, span_c.end))
 
     def similarity(self, other):
         """Make a semantic similarity estimate. The default estimate is cosine
@@ -426,6 +438,9 @@ cdef class Span:
         else:
             raise ValueError(Errors.E030)
 
+    cdef SpanC* span_c(self):
+        return self.c.get()
+
     @property
     def sents(self):
         """Obtain the sentences that contain this span. If the given span
@@ -477,10 +492,13 @@ cdef class Span:
         DOCS: https://spacy.io/api/span#ents
         """
         cdef Span ent
+        cdef SpanC* span_c = self.span_c()
+        cdef SpanC* ent_span_c
         ents = []
         for ent in self.doc.ents:
-            if ent.c.start >= self.c.start:
-                if ent.c.end <= self.c.end:
+            ent_span_c = ent.span_c()
+            if ent_span_c.start >= span_c.start:
+                if ent_span_c.end <= span_c.end:
                     ents.append(ent)
                 else:
                     break
@@ -615,11 +633,12 @@ cdef class Span:
         # This should probably be called 'head', and the other one called
         # 'gov'. But we went with 'head' elsewhere, and now we're stuck =/
         cdef int i
+        cdef SpanC* span_c = self.span_c()
         # First, we scan through the Span, and check whether there's a word
         # with head==0, i.e. a sentence root. If so, we can return it. The
         # longer the span, the more likely it contains a sentence root, and
         # in this case we return in linear time.
-        for i in range(self.c.start, self.c.end):
+        for i in range(span_c.start, span_c.end):
             if self.doc.c[i].head == 0:
                 return self.doc[i]
         # If we don't have a sentence root, we do something that's not so
@@ -630,15 +649,15 @@ cdef class Span:
         # think this should be okay.
         cdef int current_best = self.doc.length
         cdef int root = -1
-        for i in range(self.c.start, self.c.end):
-            if self.c.start <= (i+self.doc.c[i].head) < self.c.end:
+        for i in range(span_c.start, span_c.end):
+            if span_c.start <= (i+self.doc.c[i].head) < span_c.end:
                 continue
             words_to_root = _count_words_to_root(&self.doc.c[i], self.doc.length)
             if words_to_root < current_best:
                 current_best = words_to_root
                 root = i
         if root == -1:
-            return self.doc[self.c.start]
+            return self.doc[span_c.start]
         else:
             return self.doc[root]
 
@@ -654,8 +673,9 @@ cdef class Span:
             the span.
         RETURNS (Span): The newly constructed object.
         """
-        start_idx += self.c.start_char
-        end_idx += self.c.start_char
+        cdef SpanC* span_c = self.span_c()
+        start_idx += span_c.start_char
+        end_idx += span_c.start_char
         return self.doc.char_span(start_idx, end_idx, label=label, kb_id=kb_id, vector=vector)
 
     @property
@@ -736,53 +756,53 @@ cdef class Span:
 
     property start:
         def __get__(self):
-            return self.c.start
+            return self.span_c().start
 
         def __set__(self, int start):
             if start < 0:
                 raise IndexError("TODO")
-            self.c.start = start
+            self.span_c().start = start
 
     property end:
         def __get__(self):
-            return self.c.end
+            return self.span_c().end
 
         def __set__(self, int end):
             if end < 0:
                 raise IndexError("TODO")
-            self.c.end = end
+            self.span_c().end = end
 
     property start_char:
         def __get__(self):
-            return self.c.start_char
+            return self.span_c().start_char
 
         def __set__(self, int start_char):
             if start_char < 0:
                 raise IndexError("TODO")
-            self.c.start_char = start_char
+            self.span_c().start_char = start_char
 
     property end_char:
         def __get__(self):
-            return self.c.end_char
+            return self.span_c().end_char
 
         def __set__(self, int end_char):
             if end_char < 0:
                 raise IndexError("TODO")
-            self.c.end_char = end_char
+            self.span_c().end_char = end_char
 
     property label:
         def __get__(self):
-            return self.c.label
+            return self.span_c().label
 
         def __set__(self, attr_t label):
-            self.c.label = label
+            self.span_c().label = label
 
     property kb_id:
         def __get__(self):
-            return self.c.kb_id
+            return self.span_c().kb_id
 
         def __set__(self, attr_t kb_id):
-            self.c.kb_id = kb_id
+            self.span_c().kb_id = kb_id
 
     property ent_id:
         """RETURNS (uint64): The entity ID."""
diff --git a/spacy/tokens/span_group.pxd b/spacy/tokens/span_group.pxd
index 5074aa27546..6b817578a82 100644
--- a/spacy/tokens/span_group.pxd
+++ b/spacy/tokens/span_group.pxd
@@ -1,3 +1,4 @@
+from libcpp.memory cimport shared_ptr
 from libcpp.vector cimport vector
 from ..structs cimport SpanC
 
@@ -5,6 +6,6 @@ cdef class SpanGroup:
     cdef public object _doc_ref
     cdef public str name
     cdef public dict attrs
-    cdef vector[SpanC] c
+    cdef vector[shared_ptr[SpanC]] c
 
-    cdef void push_back(self, SpanC span) nogil
+    cdef void push_back(self, const shared_ptr[SpanC] &span)
diff --git a/spacy/tokens/span_group.pyx b/spacy/tokens/span_group.pyx
index 6cfa7523789..2d8b5466fcb 100644
--- a/spacy/tokens/span_group.pyx
+++ b/spacy/tokens/span_group.pyx
@@ -5,6 +5,7 @@ import srsly
 from spacy.errors import Errors
 from .span cimport Span
 from libc.stdint cimport uint64_t, uint32_t, int32_t
+from libcpp.memory cimport make_shared
 
 
 cdef class SpanGroup:
@@ -149,13 +150,13 @@ cdef class SpanGroup:
             # l: int32_t
             output["spans"].append(struct.pack(
                 ">QQQllll",
-                span.id,
-                span.kb_id,
-                span.label,
-                span.start,
-                span.end,
-                span.start_char,
-                span.end_char
+                span.get().id,
+                span.get().kb_id,
+                span.get().label,
+                span.get().start,
+                span.get().end,
+                span.get().start_char,
+                span.get().end_char
             ))
         return srsly.msgpack_dumps(output)
 
@@ -182,8 +183,8 @@ cdef class SpanGroup:
             span.end = items[4]
             span.start_char = items[5]
             span.end_char = items[6]
-            self.c.push_back(span)
+            self.c.push_back(make_shared[SpanC](span))
         return self
 
-    cdef void push_back(self, SpanC span) nogil:
+    cdef void push_back(self, const shared_ptr[SpanC] &span):
         self.c.push_back(span)


Code review comments:
diff_hunk: @@ -149,13 +150,13 @@ cdef class SpanGroup:
             # l: int32_t
             output["spans"].append(struct.pack(
                 ">QQQllll",
-                span.id,
-                span.kb_id,
-                span.label,
-                span.start,
-                span.end,
-                span.start_char,
-                span.end_char
+                span.get().id,
+                span.get().kb_id,
+                span.get().label,
+                span.get().start,
+                span.get().end,
+                span.get().start_char,
+                span.get().end_char
path: spacy/tokens/span_group.pyx
text: @user1:
Also with `span_c()`?

@author:
`SpanGroup` stores a vector of `shared_ptr[SpanC]`, so `span_c` is not available.

@author:
I'll update this with a single `get()`.

@author:
Done.