DNM unvendor all libraries #120

crusaderky · 2025-11-19T08:43:57Z

Closes Support the free-threaded build of Python 3.13/3.14 #116
Closes TypeError: 'escape_forward_slashes' is an invalid keyword argument for this function, when loading SpaCy 3.0 model (Python 3.10) #83
Closes TypeError: 'escape_forward_slashes' is an invalid keyword argument for this function, when loading SpaCy 3.5 model (Python 3.8) #89
Supersedes Remove unused-but-set-variable in ultrajsondec.c #110
Supersedes use newer unittest.mock from the standard library #111
Supersedes Upgrade Python and GitHub Actions versions #117

High level changes

In an effort to reduce the maintainance burden, unvendor all libraries except msgpack-python and upgrade them to the latest available version, with sane but reasonable version pins.

This causes the previously vendored libraries to be bumped up several years all at once. For this reason, I'd rather not merge this PR until we gather some confidence that it doesn't introduce regressions down the line.

This PR causes srsly to become a much simpler pure-python package.

Breaking changes

This PR removes srsly.msgpack, srsly.cloudpickle, srsly.ruamel_yaml, srsly.ujson. This could cause breakages downstream. The trivial fix is to simply use msgpack, cloudpickle, ruamel.yaml, and ujson directly.

Free-threading (noGIL)

This PR does not, as of today, make srsly compatible with free-threading, but it will automatically do so as soon as the below issues will be fixed upstream and new upstream releases become available:

Other changes

Bump version to 3.0, in an effort to avoid accidental bumps in downstream projects.
Fix ujson segfault in Python 3.14 (see Upgrade Python and GitHub Actions versions #117)
Introduce a minor change in json output, where dicts gain a whitespace between key and value (from {'x':1} to {'x': 1}. This should be purely cosmetic but may cause some downstream unit tests to trivially fail.
Add CI coverage for the use case where numpy is not installed

msgpack-specific changes

msgpack-python could not be unvendored. The reasons are that the latest upstream version of it

is quite old and uses a hacky patch system that offers no guarantee to remain feasible vs. future versions of msgpack, which is instead actively developed
introduces a regression where it fails to round-trip np.float64, due to it being a subclass of float
introduces a hard dependency to numpy

So instead I heavily reworked the fork that we have and added extra tests.

I also wrote from scratch a system for third-party msgpack extensions that is compatible with the previously vendored system, which is no longer available upstream.

Additionally:

Fix bug where built-in complex could not be serialized by msgpack unless numpy is installed
Fix bug where gc would remain permanently disabled if msgpack decode fails for any reason, e.g. in case of corrupted stream or if a third party extension raises.
Change exception when trying to decode a numpy object without having numpy installed from a nebulous AttributeError to a clearer ModuleNotFoundError

crusaderky · 2025-11-19T08:52:53Z

srsly/msgpack/_msgpack_numpy.py

Moved to srsly/_msgpack_numpy.py and heavily reworked

crusaderky · 2025-11-19T09:09:04Z

srsly/_msgpack_api.py

    data: The data to serialize.
    RETURNS (bytes): The serialized bytes.
    """
-    return msgpack.dumps(data, use_bin_type=True)


use_bin_type is true by default

crusaderky · 2025-11-19T09:15:27Z

requirements.txt

-cython>=0.29.1
-pytest>=4.6.5
-pytest-timeout>=1.3.3
-mock>=2.0.0,<3.0.0
-numpy>=1.15.0
-psutil


Only pytest remains, so this file makes little sense anymore

crusaderky · 2025-11-19T09:16:09Z

setup.cfg

-python_requires = >=3.9,<3.15
-setup_requires =
-    cython>=0.29.1
+python_requires = >=3.9


Removed version cap as there is very little that can break anymore with future versions. Future compatibility is now delegated to upstream libraries.

crusaderky · 2025-11-19T09:17:52Z

setup.cfg

+    catalogue>=2.0.10,<3
+    cloudpickle >=3.1.2,<4
+    msgpack >=1.1,<2
+    ruamel.yaml >=0.18.16,<1
+    ujson >=5.11.0,<6


Lower bounds are as of today. This is simply out of laziness to save the effort of pinpointing which is the minimum version of everything that works with thinc and spacy.

Higher bounds have been set very generously to reduce maintenance burden in srsly.

crusaderky · 2025-11-19T09:32:12Z

.github/workflows/tests.yml

-        shell: bash
-        run: |
-          python -m pytest --pyargs $MODULE_NAME -Werror
+        run: pytest


On 3.14t this emits

=============================== warnings summary =============================== <frozen importlib._bootstrap>:491 <frozen importlib._bootstrap>:491: RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module 'ujson', which has not declared that it can run safely without the GIL. To override this behavior and keep the GIL disabled (at your own risk), run with PYTHON_GIL=0 or -Xgil=0. -- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ======================== 85 passed, 1 warning in 0.31s =========================

I'll do a one-liner follow-up that re-adds -Werror as soon as upstream releases of msgpack and ujson become available.

ngoldbaum

Just because I'm not familiar with the package: are the tests all inherited from upstream or did you do the edits to the tests manually?

Overall awesome, I love PRs that delete a ton of code. Hopefully testing spaCy doesn't lead to discovering more issues.

ngoldbaum · 2025-11-19T16:58:52Z

srsly/_msgpack_api.py

Can you go into a little more detail than what's in the PR description into what motivated adding _MsgpackExtensions over using the old code?

I was wrong. in the opening message. The API

msgpack_encoders.register(name, cb) msgpack_deecoders.register(name, cb)

was not part of a very old version of msgpack like I thought - it was original in srsly all along.
I had to reimplement it from scratch to make it work with an unpatched msgpack, and to deal with subtle breakages with np.float64 (which is a subclass of builtin float).

srsly/_msgpack_api.py

ngoldbaum · 2025-11-19T17:05:02Z

srsly/ruamel_yaml.py

@@ -0,0 +1 @@
+from ruamel.yaml import *


I don't see how you're getting the same thing that the change in 484f519 provided before. Can you elaborate?

honnibal · 2025-11-20T12:11:50Z

FWIW, I mentioned having concerns about the library that led to this decision to vendor it and remove these unsafe branches. I found the message where I explained this to our team at the time:

So YAML has this infamous vulnerability where if you are so foolish as to write yaml.load as opposed to yaml.safe_load, you allow arbitrary code execution, as the YAML message may contain pickled Python objects.
So consider these lines: https://github.com/pycontribs/ruamel-yaml/blob/master/main.py#L1392
if issubclass(Loader, BaseLoader):
BaseConstructor.add_constructor(tag, object_constructor)
elif issubclass(Loader, SafeLoader):
SafeConstructor.add_constructor(tag, object_constructor)

Looks...alarming, right? Surely the order of conditions is wrong, so it will never enter SafeLoader 😱
Ah but wait...silly me. Why would I think SafeLoader is a subclass of BaseLoader? In fact SafeLoader has 99 base classes but BaseLoader ain't one:

class SafeLoader(Reader, Scanner, Parser, Composer, SafeConstructor, VersionedResolver):

This wasn't originally written as a public message so it's more flippant than I'd generally be --- no code is perfect, and this relates to stylistic concerns that are matters of judgment. But my concern was that future versions of the library could easily introduce a regression that affected the safe loading feature.

As I said on the call I can agree to unvendor the code as the cost/benefit analysis on the maintenance burden is different now. But I definitely want to make sure we maintain the behaviours that we had before, of forcing the library to only work in safe mode. A test that checked that the code execution features are in fact disabled would also be appreciated.

crusaderky · 2025-11-20T12:51:16Z

Not sure I understand your worry here. It seems to me that to get a code injection one needs to

deliberately bypass the srlsy API and directly use either ruamel.yaml or the backards compatibility alias srsly.yaml, and
explicitly opt in with typ='unsafe'.

I too remember the time where yaml.load was unsafe by default, but that was many years ago.

>>> import io
>>> import srsly
>>> from ruamel.yaml import YAML

>>> class C:
...     def __new__(cls):
...         print("Arbitrary code execution!")
...         return object.__new__(cls)

>>> c = C()
Arbitrary code execution!

>>> srsly.yaml_dumps(c)
RepresenterError: cannot represent an object: <__main__.C object at 0x724fd4819e10>

>>> buf = io.StringIO()
>>> yaml = YAML(typ='unsafe')
>>> yaml.dump(c, buf)
>>> buf.getvalue()
'!!python/object:__main__.C {}\n'

>>> buf.seek(0)
>>> yaml.load(buf)
Arbitrary code execution!
<__main__.C at 0x724f75c75050>

>>> yaml2 = YAML()
>>> buf.seek(0)
>>> yaml2.load(buf)
{}

>>> srsly.yaml_loads(buf.getvalue())

ValueError: Invalid YAML: could not determine a constructor for the tag 'tag:yaml.org,2002:python/object:__main__.C'
  in "<unicode string>", line 1, column 1:
    !!python/object:__main__.C {}
    ^ (line: 1)

I've added a unit test to verify this behaviour.

crusaderky · 2025-11-20T12:54:35Z

Just because I'm not familiar with the package: are the tests all inherited from upstream or did you do the edits to the tests manually?

All surviving tests are unique to srsly. All changes in them are my own edits.
I've deleted all tests that were inherited from upstream.

ngoldbaum

Did a pass over the tests. IMO it would be better if this PR only added new tests instead of changing or deleting old tests.

ngoldbaum · 2025-11-20T16:32:47Z

srsly/tests/test_yaml_api.py

+    m = Malicious()
+    buf = StringIO()
+    yaml = YAML(typ="full")
+    yaml.dump(m, buf)


It would be a little more convincing if this test used srsly.write_yaml or srsly.yaml_dumps instead.

Neither of those functions, by design, allow you to serialize an arbitrary python object.

A test demonstrating that would be more useful than what's here IMO. I searched and there are zero uses of srsly.ruamel_yaml in the explosion stack outside srsly itself. Testing the public API is what's important.

This test does not test srsly.ruamel_yaml.

This test verifies that a YAML-legal malicious payload, which can only be crafted either with unwrapped ruamel.yaml.YAML or PyYAML, cannot cause arbitrary code execution in srly.yaml_load.

I've now added a test that shows that you get a meaningful error message in srsly.write_yaml when trying to write arbitrary objects, and removed the lines that use ruamel.yaml.YAML() to load the payload back in order to prevent confusion.

I searched and there are zero uses of srsly.ruamel_yaml in the explosion stack outside srsly itself.

Are you suggesting to not introduce the dummy srsly/ruamel_yaml.py etc. backwards compatibiity modules, incurring in a slight risk that third-party packages may (trivially) break?

ngoldbaum · 2025-11-20T16:35:20Z

srsly/tests/test_json_api.py

-    with pytest.raises(TypeError):
-        s = json_dumps(f)
+    with pytest.raises(TypeError, match="is not JSON serializable"):
+        s = json_dumps({1, 2})


why not keep the old content too?

Maybe you wanted to get rid of numpy as a dependency? You could leave it as a test dependency.

numpy was never a runtime dependency. It was however a test dependency and this change (plus changes in the msgpack tests) makes it become optional.
From my point of view there is no difference between a set and a np.float32 - they're both types that json does not understand. So might as well simplify.

I did not want to keep it as a test dependency because this PR introduces a use case (see the gh action changes) that proves that you can actually do everything except msgpack-numpy without having numpy installed. This was prompted by noticing that if numpy is not installed the old code was failing to serialize builtin complex.

I understand and I appreciate that having tests running without numpy installed is good.

I still think it would be easier to review this PR if you only added new tests instead of changing or deleting old ones. As you have it here it's harder to verify that everything still works as it used to.

I've reinstated the old test alongside the new one.

srsly/tests/test_json_api.py

ngoldbaum · 2025-11-20T16:39:25Z

srsly/tests/test_msgpack_api.py

-    when serializing datetime objects, the error should be msgpack's TypeError,
-    not a "'np' is not defined error")."""
-    with pytest.raises(TypeError):
-        msgpack_loads(msgpack_dumps(datetime.datetime.now()))


why delete this test?

The test was using a very hacky monkey-patch design that really didn't work well with a separate msgpack library outside of our control. Instead of it I'm just running the whole test suite without numpy installed with a new gh actions job.

Couldn't you leave it, remove the monkeypatching, and mark it to be skipped if NumPy isn't importable?

This specific test had just been renamed. I've now reinstated the previous name.

ngoldbaum · 2025-11-20T16:39:53Z

srsly/tests/test_msgpack_api.py

        if "__custom__" in obj:
            return CustomObject(obj["__custom__"])
-        return obj if chain is None else chain(obj)


was chain unused or something? not clear why it got deleted.

It was probably used by the older code but to the best of my understanding it was not useful the way the callback API is used in all examples in the codebase, so I just simplified it. The old callbacks will continue working as long as they have chain=None in the signature. This may be disproved by dependency packages but I have my doubts.

Again, there's no reason to delete old testing code IMO. It just makes this PR harder to review and verify that everything will continue to work as it used to.

but... chain is always None if you define a callback with chain=None and never pass a chain parameter to it.

ngoldbaum · 2025-11-20T16:41:13Z

srsly/tests/msgpack/test_numpy.py

-        assert_equal(type(self.encode_decode(b"foo")), bytes)
-
-    def test_str(self):
-        assert_equal(type(self.encode_decode("foo")), bytes)


why did these get deleted?

This is thoroughly tested by the upstream msgpack tests.

Reinstated.

ngoldbaum · 2025-11-20T16:41:30Z

srsly/tests/msgpack/test_numpy.py

-    def test_chain(self):
-        x = ThirdParty(foo=b"test marshal/unmarshal")
-        x_rec = self.encode_decode_thirdparty(x)
-        self.assertEqual(x, x_rec)


why did this get deleted?

This is already tested by test_msgpack_api.py::test_msgpack_custom_encoder_decoder

Reinstated.

ngoldbaum · 2025-11-20T16:43:24Z

srsly/tests/test_msgpack_numpy.py

-        assert_equal(type(self.encode_decode("foo")), bytes)
+    def encode_decode(self, x):
+        x_enc = msgpack_dumps(x)
+        return msgpack_loads(x_enc)


I think I understand why encode_decode changed: now it only uses public APIs. But why did all the other tests get deleted?

The other tests were redundant either with the upstream tests in msgpack or with test_msgpack_api.py.

Reinstated.

ngoldbaum · 2025-11-20T16:43:34Z

srsly/tests/msgpack/test_numpy.py

-    def test_chain(self):
-        x = ThirdParty(foo=b"test marshal/unmarshal")
-        x_rec = self.encode_decode_thirdparty(x)
-        self.assertEqual(x, x_rec)


why was this test deleted?

No more chain parameter is necessary because of how _MsgpackExtensions._run works. There may be some very obscure use in the dependencies that disproves me.

Reinstated.

ngoldbaum · 2025-11-20T16:47:38Z

srsly/tests/test_msgpack_api.py

@@ -91,10 +141,70 @@ def deserialize_obj(obj, chain=None):
    assert new_data["a"] == 123
    assert isinstance(new_data["b"], CustomObject)
    assert new_data["b"].value == {"foo": "bar"}
-    # Test that it also works with combinations of encoders/decoders (e.g. numpy)
-    data = {"a": numpy.zeros((1, 2, 3)), "b": CustomObject({"foo": "bar"})}


why change this not to use numpy? You could for example change this test to be parameterized by an input argument and have one of the arguments be conditionally marked such that the test is skipped if numpy isn't importable. That way you get both the content of the test as you have it in this PR and the old test.

The purpose of this test is to verify that registering a custom encoder/decoder does not break the builtin ones. So complex and np.ndarray are equivalent, with the former being able to run when numpy is not installed.

crusaderky · 2025-11-21T12:38:29Z

@ngoldbaum I've reinstated a bunch of tests where feasible.

crusaderky · 2025-11-21T12:50:32Z

srsly/tests/test_msgpack_numpy.py


    def encode_thirdparty(self, obj):
-        return dict(__thirdparty__=True, foo=obj.foo)
+        if isinstance(obj, ThirdParty):
+            return {b"__thirdparty__": True, b"foo": obj.foo}


Note: on first sight, this may look like a major breaking change, where str keys are no longer converted to bytes upon serialization.
This however is an artefact of the old unit test, which was using the direct msgpack API msgpack.packb(x, default=self.encode_thirdparty, use_bin_type=use_bin_type) (see old lines 28:31).
These options have never been accessible from srsly.msgpack_dumps. With the srsly API, in this as well as the old version, str round-trips to str and bytes round-trips to bytes.

crusaderky · 2025-11-24T17:12:30Z

As discussed offline with @ngoldbaum , I've removed the legacy subpackages srsly.msgpack, srsly.cloudpickle, srsly.ruamel_yaml, and srsly.ujson. This is a breaking change.

crusaderky commented Nov 19, 2025

View reviewed changes

srsly/msgpack/_msgpack_numpy.py Outdated

Copy link

Contributor Author

crusaderky Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to srsly/_msgpack_numpy.py and heavily reworked

crusaderky commented Nov 19, 2025

View reviewed changes

crusaderky force-pushed the ft branch from 9387da5 to 5703a64 Compare November 19, 2025 09:11

crusaderky commented Nov 19, 2025

View reviewed changes

crusaderky mentioned this pull request Nov 19, 2025

Upgrade Python and GitHub Actions versions #117

Open

Unvendor libraries

11c9aa9

crusaderky force-pushed the ft branch from 5703a64 to 11c9aa9 Compare November 19, 2025 11:29

ngoldbaum reviewed Nov 19, 2025

View reviewed changes

crusaderky added 2 commits November 20, 2025 12:49

Tweak msgpack

11d63a2

Test that YAML is safe

921ef77

ngoldbaum reviewed Nov 20, 2025

View reviewed changes

crusaderky added 2 commits November 21, 2025 11:45

Test srsly.yaml_dump vs. arbitrary objects; clarify test_yaml_safe

8a5b00b

Fix backwards compatibility imports

6483817

crusaderky added 2 commits November 21, 2025 12:41

Reduce diff to test_msgpack_numpy

d8b93b9

Reinstated bunch of tests

f71c206

crusaderky force-pushed the ft branch from fab1e2a to f71c206 Compare November 21, 2025 12:41

crusaderky commented Nov 21, 2025

View reviewed changes

crusaderky mentioned this pull request Nov 21, 2025

Add spaCy scientific-python/upload-nightly-action#146

Open

7 tasks

crusaderky added 2 commits November 24, 2025 17:06

YAML test clarification

257f9f1

BREAKING: Remove legacy embedded modules

9940c65

crusaderky mentioned this pull request Nov 27, 2025

Python 3.14 support explosion/confection#69

Merged

Tweak ccomment

cfbd084

DNM unvendor all libraries #120

Are you sure you want to change the base?

DNM unvendor all libraries #120

Conversation

crusaderky commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High level changes

Breaking changes

Free-threading (noGIL)

Other changes

msgpack-specific changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusaderky Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusaderky Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

honnibal commented Nov 20, 2025

Uh oh!

crusaderky commented Nov 20, 2025

Uh oh!

crusaderky commented Nov 20, 2025

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crusaderky commented Nov 19, 2025 •

edited

Loading

crusaderky Nov 19, 2025 •

edited

Loading

crusaderky Nov 19, 2025 •

edited

Loading