From 040230a185672b0827ccedb8b89aece0c0cd3a32 Mon Sep 17 00:00:00 2001 From: James Hilliard Date: Wed, 21 Oct 2026 11:35:02 -0600 Subject: [PATCH] Support Python 3.15 and free-threaded builds --- .github/workflows/build_and_upload.yml | 2 +- .github/workflows/tests.yml | 2 +- MANIFEST.in | 2 +- pycrfsuite/_crfsuite_compat.h | 10 ++ pycrfsuite/_pycrfsuite.pyx | 164 ++++++++++++++++++------- pycrfsuite/tagger_wrapper.hpp | 14 ++- pycrfsuite/trainer_wrapper.cpp | 12 ++ pyproject.toml | 3 +- setup.py | 9 ++ tests/test_itemsequence.py | 22 ++++ tests/test_misc.py | 30 +++++ tests/test_tagger.py | 28 ++++- tests/test_trainer.py | 28 +++++ tox.ini | 5 +- 14 files changed, 273 insertions(+), 58 deletions(-) create mode 100644 pycrfsuite/_crfsuite_compat.h diff --git a/.github/workflows/build_and_upload.yml b/.github/workflows/build_and_upload.yml index 42df75f..ed1fdc8 100644 --- a/.github/workflows/build_and_upload.yml +++ b/.github/workflows/build_and_upload.yml @@ -20,7 +20,7 @@ jobs: with: submodules: true - name: "Build wheels" - uses: "pypa/cibuildwheel@v3.3.0" + uses: "pypa/cibuildwheel@v4.3.3" env: CIBW_SKIP: "pp* cp38-* cp39-* cp310-win_arm64" CIBW_TEST_REQUIRES: "pytest" diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a3304d0..98cfc99 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -13,7 +13,7 @@ jobs: strategy: matrix: - python-version: ["3.10", "3.11", "3.12", "3.13", "3.14"] + python-version: ["3.10", "3.11", "3.12", "3.13", "3.14", "3.14t", "3.15", "3.15t"] os: [ubuntu-latest, ubuntu-24.04-arm, macos-latest, windows-latest, windows-11-arm] exclude: # CPython has no official win_arm64 release before 3.11. diff --git a/MANIFEST.in b/MANIFEST.in index e6c3c2a..ae16d5c 100644 --- a/MANIFEST.in +++ b/MANIFEST.in @@ -6,4 +6,4 @@ include update_cpp.sh recursive-include crfsuite * recursive-include liblbfgs * recursive-include tests *.py -recursive-include pycrfsuite *.py *.pxd *.pyx *.cpp *.hpp +recursive-include pycrfsuite *.py *.pxd *.pyx *.cpp *.h *.hpp diff --git a/pycrfsuite/_crfsuite_compat.h b/pycrfsuite/_crfsuite_compat.h new file mode 100644 index 0000000..41d601f --- /dev/null +++ b/pycrfsuite/_crfsuite_compat.h @@ -0,0 +1,10 @@ +#ifndef PYCRFSUITE_COMPAT_H +#define PYCRFSUITE_COMPAT_H + +#ifndef __cplusplus +#include +int pycrfsuite_rand(void); +#define rand pycrfsuite_rand +#endif + +#endif diff --git a/pycrfsuite/_pycrfsuite.pyx b/pycrfsuite/_pycrfsuite.pyx index 647f203..970af06 100644 --- a/pycrfsuite/_pycrfsuite.pyx +++ b/pycrfsuite/_pycrfsuite.pyx @@ -2,6 +2,7 @@ # cython: c_string_type=str # cython: c_string_encoding=utf-8 # cython: profile=False +# cython: freethreading_compatible=True # distutils: language=c++ from . cimport crfsuite_api from libcpp.string cimport string @@ -10,6 +11,7 @@ import sys import os import contextlib import tempfile +from threading import RLock from pycrfsuite import _dumpparser from pycrfsuite import _logparser @@ -101,9 +103,12 @@ cdef extern crfsuite_api.ItemSequence to_seq(pyseq) except+: "string_prefix" s prepended to each key. """ cdef crfsuite_api.ItemSequence c_seq + cdef ItemSequence sequence if isinstance(pyseq, ItemSequence): - c_seq = (pyseq).c_seq + sequence = pyseq + with sequence._lock: + c_seq = sequence.c_seq else: for x in pyseq: c_seq.push_back(to_item(x)) @@ -155,9 +160,16 @@ cdef class ItemSequence(object): """ cdef crfsuite_api.ItemSequence c_seq + # Calling __init__ again can replace the contents of an existing sequence. + cdef object _lock + + def __cinit__(self): + self._lock = RLock() def __init__(self, pyseq): - self.c_seq = to_seq(pyseq) + cdef crfsuite_api.ItemSequence c_seq = to_seq(pyseq) + with self._lock: + self.c_seq = c_seq def items(self): """ @@ -173,7 +185,7 @@ cdef class ItemSequence(object): cdef bytes key seq = [] - for c_item in self.c_seq: + for c_item in to_seq(self): x = {} for c_attr in c_item: # Always decode keys from utf-8. It means binary keys are @@ -188,7 +200,8 @@ cdef class ItemSequence(object): return seq def __len__(self): - return self.c_seq.size() + with self._lock: + return self.c_seq.size() def __repr__(self): return "" % len(self) @@ -219,6 +232,8 @@ cdef class BaseTrainer(object): """ cdef crfsuite_api.Trainer c_trainer + cdef object _lock + cdef bint _training _PARAMETER_TYPES = { 'feature.minfreq': float, @@ -261,6 +276,7 @@ cdef class BaseTrainer(object): self.verbose = verbose def __cinit__(self): + self._lock = RLock() # setup message handler self.c_trainer.set_handler(self, self._on_message) @@ -268,6 +284,10 @@ cdef class BaseTrainer(object): self.c_trainer.select("lbfgs".encode('ascii'), "crf1d".encode('ascii')) self.c_trainer._init_hack() + cdef _check_not_training(self): + if self._training: + raise RuntimeError("Cannot modify a trainer during training") + cdef _on_message(self, string message): self.message(message) @@ -309,7 +329,11 @@ cdef class BaseTrainer(object): The group number of the instance. Group numbers are used to select subset of data for heldout evaluation. """ - self.c_trainer.append(to_seq(xseq), yseq, group) + cdef crfsuite_api.ItemSequence c_seq = to_seq(xseq) + cdef crfsuite_api.StringList c_labels = yseq + with self._lock: + self._check_not_training() + self.c_trainer.append(c_seq, c_labels, group) def select(self, algorithm, type='crf1d'): """ @@ -331,10 +355,14 @@ cdef class BaseTrainer(object): """ algorithm = algorithm.lower() algorithm = self._ALGORITHM_ALIASES.get(algorithm, algorithm) - if not self.c_trainer.select(algorithm.encode('ascii'), type.encode('ascii')): - raise ValueError( - "Bad arguments: algorithm=%r, type=%r" % (algorithm, type) - ) + cdef string c_algorithm = algorithm.encode('ascii') + cdef string c_type = type.encode('ascii') + with self._lock: + self._check_not_training() + if not self.c_trainer.select(c_algorithm, c_type): + raise ValueError( + "Bad arguments: algorithm=%r, type=%r" % (algorithm, type) + ) def train(self, model, int holdout=-1): """ @@ -342,6 +370,10 @@ cdef class BaseTrainer(object): This function starts the training algorithm with the data set given by :meth:`Trainer.append` method. + Callbacks may inspect this trainer, but changing its training data, + algorithm or parameters, or calling ``train()`` again, raises + ``RuntimeError``. + Parameters ---------- model : string @@ -355,10 +387,17 @@ cdef class BaseTrainer(object): for training, but for holdout evaluation. Default value is -1, meaning "use all instances for training". """ - self._before_train() - status_code = self.c_trainer.train(model, holdout) - if status_code != crfsuite_api.CRFSUITE_SUCCESS: - raise CRFSuiteError(status_code) + cdef string c_model = model + with self._lock: + self._check_not_training() + self._training = True + try: + self._before_train() + status_code = self.c_trainer.train(c_model, holdout) + if status_code != crfsuite_api.CRFSUITE_SUCCESS: + raise CRFSuiteError(status_code) + finally: + self._training = False def params(self): """ @@ -375,7 +414,8 @@ cdef class BaseTrainer(object): graphical model and training algorithm. """ - return self.c_trainer.params() + with self._lock: + return self.c_trainer.params() def set_params(self, params): """ @@ -386,8 +426,9 @@ cdef class BaseTrainer(object): params : dict A dict with parameters ``{name: value}`` """ - for key, value in params.items(): - self.set(key, value) + with self._lock: + for key, value in params.items(): + self.set(key, value) def get_params(self): """ @@ -400,7 +441,8 @@ cdef class BaseTrainer(object): with all trainer parameters. """ # params = self.params() - return dict((name, self.get(name)) for name in self.params()) + with self._lock: + return dict((name, self.get(name)) for name in self.params()) def set(self, name, value): """ @@ -418,7 +460,11 @@ cdef class BaseTrainer(object): """ if isinstance(value, bool): value = int(value) - self.c_trainer.set(name, str(value)) + cdef string c_name = name + cdef string c_value = str(value) + with self._lock: + self._check_not_training() + self.c_trainer.set(c_name, c_value) def get(self, name): """ @@ -431,7 +477,10 @@ cdef class BaseTrainer(object): name : string The parameter name. """ - return self._cast_parameter(name, self.c_trainer.get(name)) + cdef string c_name = name + with self._lock: + value = self.c_trainer.get(c_name) + return self._cast_parameter(name, value) def help(self, name): """ @@ -451,15 +500,19 @@ cdef class BaseTrainer(object): The description (help message) of the parameter. """ - if name not in self.params(): - # c_trainer.help(name) segfaults without this workaround; - # see https://github.com/chokkan/crfsuite/pull/21 - raise ValueError("Parameter not found: %s" % name) - return self.c_trainer.help(name) + cdef string c_name = name + with self._lock: + if name not in self.params(): + # c_trainer.help(name) segfaults without this workaround; + # see https://github.com/chokkan/crfsuite/pull/21 + raise ValueError("Parameter not found: %s" % name) + return self.c_trainer.help(c_name) def clear(self): """ Remove all instances in the data set. """ - self.c_trainer.clear() + with self._lock: + self._check_not_training() + self.c_trainer.clear() def _cast_parameter(self, name, value): if name in self._PARAMETER_TYPES: @@ -554,6 +607,10 @@ cdef class Tagger(object): input sequences using a model. """ cdef crfsuite_api.Tagger c_tagger + cdef object _lock + + def __cinit__(self): + self._lock = RLock() def open(self, name): """ @@ -569,8 +626,10 @@ cdef class Tagger(object): # may segfault if the file is invalid. # See https://github.com/chokkan/crfsuite/pull/24 self._check_model(name) - if not self.c_tagger.open(name): - raise ValueError("Error opening model file %r" % name) + cdef string c_name = name + with self._lock: + if not self.c_tagger.open(c_name): + raise ValueError("Error opening model file %r" % name) return contextlib.closing(self) def open_inmemory(self, bytes value): @@ -585,15 +644,17 @@ cdef class Tagger(object): """ self._check_inmemory_model(value) cdef const char *v = value - if not self.c_tagger.open(v, len(value)): - raise ValueError("Error opening model") + with self._lock: + if not self.c_tagger.open(v, len(value)): + raise ValueError("Error opening model") return contextlib.closing(self) def close(self): """ Close the model. """ - self.c_tagger.close() + with self._lock: + self.c_tagger.close() def labels(self): """ @@ -604,7 +665,8 @@ cdef class Tagger(object): list of strings The list of labels in the model. """ - return self.c_tagger.labels() + with self._lock: + return self.c_tagger.labels() def tag(self, xseq=None): """ @@ -626,10 +688,10 @@ cdef class Tagger(object): list of strings The label sequence predicted. """ - if xseq is not None: - self.set(xseq) - - return self.c_tagger.viterbi() + with self._lock: + if xseq is not None: + self.set(xseq) + return self.c_tagger.viterbi() def probability(self, yseq): """ @@ -647,7 +709,9 @@ cdef class Tagger(object): float The probability ``P(yseq|xseq)``. """ - return self.c_tagger.probability(yseq) + cdef crfsuite_api.StringList c_labels = yseq + with self._lock: + return self.c_tagger.probability(c_labels) def marginal(self, y, pos): """ @@ -668,7 +732,10 @@ cdef class Tagger(object): float The marginal probability of the label ``y`` at position ``t``. """ - return self.c_tagger.marginal(y, pos) + cdef string c_y = y + cdef int c_pos = pos + with self._lock: + return self.c_tagger.marginal(c_y, c_pos) cpdef extern set(self, xseq) except +: """ @@ -676,6 +743,9 @@ cdef class Tagger(object): :meth:`Tagger.tag`, :meth:`Tagger.probability` and :meth:`Tagger.marginal` methods. + When sharing a tagger across threads, use an external lock around + ``set()`` and the subsequent calls that use that sequence. + Parameters ---------- xseq : item sequence @@ -685,7 +755,9 @@ cdef class Tagger(object): in :class:`~ItemSequence` docs. """ - self.c_tagger.set(to_seq(xseq)) + cdef crfsuite_api.ItemSequence c_seq = to_seq(xseq) + with self._lock: + self.c_tagger.set(c_seq) def dump(self, filename=None): """ @@ -698,16 +770,14 @@ cdef class Tagger(object): If None, the model is dumped to stdout. """ if filename is None: - self.c_tagger.dump(os.dup(sys.stdout.fileno())) + fd = os.dup(sys.stdout.fileno()) else: fd = os.open(filename, os.O_CREAT | os.O_WRONLY) - try: + try: + with self._lock: self.c_tagger.dump(fd) - finally: - try: - os.close(fd) - except OSError: - pass # already closed by Tagger::dump + finally: + os.close(fd) def info(self): """ @@ -716,11 +786,13 @@ cdef class Tagger(object): parser = _dumpparser.CRFsuiteDumpParser() fd, name = tempfile.mkstemp() try: - self.c_tagger.dump(fd) + with self._lock: + self.c_tagger.dump(fd) with open(name, 'rb') as f: for line in f: parser.feed(line.decode('utf8')) finally: + os.close(fd) try: os.unlink(name) except OSError: diff --git a/pycrfsuite/tagger_wrapper.hpp b/pycrfsuite/tagger_wrapper.hpp index 4eda608..f115e1e 100644 --- a/pycrfsuite/tagger_wrapper.hpp +++ b/pycrfsuite/tagger_wrapper.hpp @@ -5,6 +5,11 @@ #include #include #include "crfsuite_api.hpp" +#ifdef _WIN32 +#include +#else +#include +#endif namespace CRFSuiteWrapper @@ -23,8 +28,15 @@ class Tagger : public CRFSuite::Tagger throw std::runtime_error("Tagger is closed"); } - FILE* file = fdopen(fileno, "w"); + // Python owns fileno; closing it here would race with descriptor reuse + // in other threads before Python's finally block closes it again. + int fd = dup(fileno); + if (fd == -1) { + throw std::runtime_error("Can't duplicate file descriptor"); + } + FILE* file = fdopen(fd, "w"); if (!file){ + ::close(fd); throw std::runtime_error("Can't open file"); } diff --git a/pycrfsuite/trainer_wrapper.cpp b/pycrfsuite/trainer_wrapper.cpp index cae0d6b..dea3cd1 100644 --- a/pycrfsuite/trainer_wrapper.cpp +++ b/pycrfsuite/trainer_wrapper.cpp @@ -2,6 +2,8 @@ #include "Python.h" #include "trainer_wrapper.hpp" #include +#include +#include namespace CRFSuiteWrapper { @@ -39,3 +41,13 @@ void Trainer::_init_hack() } + +#ifdef Py_GIL_DISABLED +// Protect libc's RNG state without changing its sequence. +extern "C" int pycrfsuite_rand(void) +{ + static std::mutex mutex; + std::lock_guard lock(mutex); + return std::rand(); +} +#endif diff --git a/pyproject.toml b/pyproject.toml index 5bd9410..a644a62 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -22,6 +22,7 @@ classifiers = [ "Programming Language :: Python :: 3.12", "Programming Language :: Python :: 3.13", "Programming Language :: Python :: 3.14", + "Programming Language :: Python :: 3.15", "Topic :: Software Development", "Topic :: Software Development :: Libraries :: Python Modules", "Topic :: Scientific/Engineering", @@ -40,7 +41,7 @@ dev = ["tox", ] [build-system] -requires = ["setuptools>=42", "wheel", "cython"] +requires = ["setuptools>=75.7", "wheel", "cython>=3.1"] build-backend = "setuptools.build_meta" diff --git a/setup.py b/setup.py index ca958c4..9e33fc0 100755 --- a/setup.py +++ b/setup.py @@ -1,6 +1,7 @@ #!/usr/bin/env python import glob import sys +import sysconfig from distutils.command.build_ext import build_ext from Cython.Build import cythonize @@ -30,6 +31,14 @@ class build_ext_check_gcc(build_ext): def build_extensions(self): c = self.compiler + if sysconfig.get_config_var("Py_GIL_DISABLED"): + for extension in self.extensions: + extension.extra_compile_args.extend( + ["/FI_crfsuite_compat.h"] + if c.compiler_type == "msvc" + else ["-include", "_crfsuite_compat.h"] + ) + _compile = c._compile def c_compile(obj, src, ext, cc_args, extra_postargs, pp_opts): diff --git a/tests/test_itemsequence.py b/tests/test_itemsequence.py index 5c33201..0d7bd27 100644 --- a/tests/test_itemsequence.py +++ b/tests/test_itemsequence.py @@ -1,3 +1,6 @@ +from concurrent.futures import ThreadPoolExecutor +from threading import Barrier + import pytest import pycrfsuite @@ -9,6 +12,25 @@ def test_basic(): assert seq.items() == [] +def test_concurrent_reinitialization(): + seq = pycrfsuite.ItemSequence([]) + items = [{"foo": 1.0}] * 80 + barrier = Barrier(4) + + def access(index): + barrier.wait(timeout=30) + for iteration in range(500): + if index == 0: + seq.__init__(items if iteration % 2 else []) + else: + assert seq.items() in ([], items) + assert pycrfsuite.ItemSequence(seq).items() in ([], items) + assert len(seq) in (0, len(items)) + + with ThreadPoolExecutor(max_workers=4) as pool: + list(pool.map(access, range(4))) + + def test_lists(): seq = pycrfsuite.ItemSequence([["foo", "bar"], ["bar", "baz"]]) assert len(seq) == 2 diff --git a/tests/test_misc.py b/tests/test_misc.py index 079256f..98fb4ee 100644 --- a/tests/test_misc.py +++ b/tests/test_misc.py @@ -1,4 +1,34 @@ +import os +import subprocess +import sys +import sysconfig + +import pytest + + def test_version(): from pycrfsuite import CRFSUITE_VERSION assert bool(CRFSUITE_VERSION), CRFSUITE_VERSION + + +@pytest.mark.skipif( + not sysconfig.get_config_var("Py_GIL_DISABLED"), + reason="requires a free-threaded interpreter", +) +def test_import_preserves_free_threading(): + env = os.environ.copy() + env.pop("PYTHON_GIL", None) + subprocess.run( + [ + sys.executable, + "-W", + "error", + "-c", + "import sys; assert not sys._is_gil_enabled(); " + "import pycrfsuite; assert not sys._is_gil_enabled()", + ], + env=env, + check=True, + timeout=30, + ) diff --git a/tests/test_tagger.py b/tests/test_tagger.py index 3ebb021..b57a906 100644 --- a/tests/test_tagger.py +++ b/tests/test_tagger.py @@ -1,3 +1,6 @@ +from concurrent.futures import ThreadPoolExecutor +from threading import Barrier + import pytest from pycrfsuite import ItemSequence, Tagger, Trainer @@ -70,13 +73,11 @@ def test_open_inmemory_invalid(): tagger.open_inmemory(b"lCRFabc") -@pytest.mark.xfail( - reason="see https://github.com/scrapinghub/python-crfsuite/issues/28", run=False -) -def test_tag_not_opened(xseq): +@pytest.mark.parametrize("method", ["set", "tag"]) +def test_tagger_not_opened(xseq, method): tagger = Tagger() - with pytest.raises(Exception): - tagger.tag(xseq) + with pytest.raises(ValueError, match="^The tagger is not opened$"): + getattr(tagger, method)(xseq) def test_tag(model_filename, xseq, yseq): @@ -84,6 +85,21 @@ def test_tag(model_filename, xseq, yseq): assert tagger.tag(xseq) == yseq +def test_tag_shared_between_threads(model_filename, xseq): + barrier = Barrier(4) + with Tagger().open(model_filename) as tagger: + sequences = [ItemSequence(xseq[:length]) for length in (1, 3, 7, 11)] + expected = [tagger.tag(sequence) for sequence in sequences] + + def tag(index): + barrier.wait(timeout=30) + for _ in range(500): + assert tagger.tag(sequences[index]) == expected[index] + + with ThreadPoolExecutor(max_workers=4) as pool: + list(pool.map(tag, range(4))) + + def test_tag_item_sequence(model_filename, xseq, yseq): with Tagger().open(model_filename) as tagger: assert tagger.tag(ItemSequence(xseq)) == yseq diff --git a/tests/test_trainer.py b/tests/test_trainer.py index 4eafc5d..de385ef 100644 --- a/tests/test_trainer.py +++ b/tests/test_trainer.py @@ -76,6 +76,34 @@ def message(self, message): trainer.train(model_filename) +@pytest.mark.parametrize("mutation", ["append", "clear", "select", "set", "train"]) +def test_training_callback_rejects_mutation(tmpdir, xseq, yseq, mutation): + model_filename = str(tmpdir.join("model.crfsuite")) + + class CheckingTrainer(Trainer): + checked = False + + def message(self, message): + if not self.checked: + self.checked = True + assert self.get("max_iterations") == 2 + args = { + "append": (xseq, yseq), + "clear": (), + "select": ("lbfgs",), + "set": ("max_iterations", 1), + "train": (model_filename,), + } + with pytest.raises(RuntimeError, match="during training"): + getattr(self, mutation)(*args[mutation]) + + trainer = CheckingTrainer(params={"max_iterations": 2}, verbose=False) + trainer.append(xseq, yseq) + trainer.train(model_filename) + assert trainer.checked + trainer.clear() + + def test_trainer_select_raises_error(): trainer = Trainer() with pytest.raises(ValueError): diff --git a/tox.ini b/tox.ini index 45d52b7..babb3da 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,5 @@ [tox] -envlist = py310,py311,py312,py313,py314 +envlist = py310,py311,py312,py313,py314,py314t,py315,py315t [gh-actions] python = @@ -8,6 +8,9 @@ python = 3.12: py312 3.13: py313 3.14: py314 + 3.14t: py314t + 3.15: py315 + 3.15t: py315t [testenv] changedir = {envtmpdir}