update from review

[MiXaiLL76]

No description provided.

[espressolee]

I checked exact head b6640436b9a8b7c1ca643923f6f2b00a370495d9 on macOS arm64 with CPython 3.14.6t.

The three smaller review updates look reasonable: -k "TestDatasetConcurrency" still includes test_concurrent_evaluator_and_mask_operations because that test is a method of the class; the conditional pybind11 requirements preserve 2.13 support before Python 3.14 while selecting 3.x for 3.14/3.14t; and the const auto& bindings avoid copies while iterating the local annotation snapshot.

However, this head does not resolve the blocking liveness issue from #127. The cache payload now has safe shared ownership, but publication still compares against the dataset-wide annotation_version, and every append_ref() still increments that global version. An unrelated-key append from each conversion hook therefore invalidates every attempt and keeps the retry loop running.

Exact-head controls:

• no-GIL import succeeded and kept sys._is_gil_enabled() false;
• tests/test_cpp_safety_regressions.py: 17/17 PASS;
• one-shot unrelated-key mutation: returned 5/5, with two conversion-hook calls each;
• repeated unrelated-key mutation: timed out 5/5 at the external 3-second bound, with about 1.3M to 4.37M hook calls observed before termination.

This is the same causal split as the original review: a single invalidation converges, while repeated writes to an unrelated key prevent convergence. The shared_ptr change addresses cache-entry lifetime, not the global invalidation condition.

Please use a per-key append version/source identity plus a separate global replacement epoch for clean() / load_tuple(), or an equivalent condition under which unrelated-key writes cannot invalidate publication for the requested key. Continuous mutation of the requested key may need retry or abstention semantics; unrelated-key mutation should not starve the read.

[Copilot]

🟡 Changes recommended

CI no longer runs an existing deadlock-regression test (test_concurrent_evaluator_and_mask_operations), reducing coverage for concurrency safety.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates build dependencies and refines the C++ dataset caching/concurrency implementation, along with a small CI adjustment, to improve compatibility (notably around Python 3.14) and reduce cache-related concurrency hazards.

Changes:

• Split pybind11 build requirements by Python version to support CPython 3.14+.
• Rework LightweightDataset’s C++ annotation cache to store immutable payloads behind shared_ptr and adjust move semantics/documentation accordingly.
• Update the CI pytest selection for the native stress regressions job.

File summaries

File	Description
pyproject.toml	Adds Python-version-conditional pybind11 build requirements.
csrc/faster_eval_api/coco_eval/dataset.h	Changes cache entry type to shared_ptr<const vector<...>> and tightens move/assignment semantics.
csrc/faster_eval_api/coco_eval/dataset.cpp	Implements shared_ptr-backed cache reads/writes and reduces lock hold time; minor move-iterator optimization in merge path.
.github/workflows/unittest.yml	Narrows the pytest -k selector for the native stress regression run.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[espressolee]

I don't think this one is right. test_concurrent_evaluator_and_mask_operations is a method of TestDatasetConcurrency:

class TestDatasetConcurrency:
    ...
    def test_concurrent_evaluator_and_mask_operations(self):

-k matches against the full node id, so -k "TestDatasetConcurrency" already selects it. The removed term was redundant with the first one and this job's selection is unchanged — no coverage is lost here.

Easy to confirm either way:

pytest -q test_cpp_safety_regressions.py -k "TestDatasetConcurrency" --collect-only

against the old and new selectors should list the same set.

[Copilot]

🔵 Needs a closer look

The pybind11 requirement branches must preserve the existing <4 upper bound.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pyproject.toml:12

• Both marker branches drop the existing <4 cap, so isolated builds may automatically select a future incompatible major release. Preserve the upper bound while splitting the minimum version by Python version.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Balanced