fix: make Tests CI green (py3.9 collection + Hermes classifier determinism)#192
Open
saschabuehrle wants to merge 2 commits into
Open
fix: make Tests CI green (py3.9 collection + Hermes classifier determinism)#192saschabuehrle wants to merge 2 commits into
saschabuehrle wants to merge 2 commits into
Conversation
tests/test_provider_tool_retry.py used 'str | None' in method signatures, which Python 3.9 evaluates at definition time and rejects with 'TypeError: unsupported operand type(s) for |'. This broke test collection on the 3.9 CI matrix leg. Add 'from __future__ import annotations' to defer annotation evaluation, matching the rest of the suite.
The classifier scored domains with naive substring matching, so short keywords matched inside unrelated words -- e.g. 'ci' (ops) matched inside 'financial', tying with the finance hit and winning on dict order. This made test_high_stakes_domains_are_detected fail nondeterministically depending on PYTHONHASHSEED (which keyword next(iter(set)) picked). Use word-boundary regex matching so 'ci'/'api'/'law' no longer match inside 'financial'/'always'; multi-word keywords still match. The classifier is now correct and the test deterministic across seeds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The Tests badge on
mainhas been red. Two independent failures in thetest.ymlPython matrix:Collection error (Python 3.9 leg) —
tests/test_provider_tool_retry.pyTypeError: unsupported operand type(s) for |: 'type' and 'NoneType'The file used
str | Nonein method signatures. Python 3.9 evaluates annotations at definition time, so this raised at import → the whole file failed to collect. (3.10+ legs were unaffected.)Nondeterministic failure —
tests/integrations/test_hermes_classifier.py::test_high_stakes_domains_are_detectedAssertionError: assert 'ops' == 'finance'The test picks a keyword via
next(iter(DOMAIN_KEYWORDS[domain]))on a set, so the choice depends onPYTHONHASHSEEDand varies per CI run. When it picked"financial", the classifier's naive substring match"ci" in loweredmatched theciinside "financial", giving the ops domain a tying hit that won on dict order. Red only on unlucky seeds.Fix
from __future__ import annotationstotest_provider_tool_retry.py(defers annotation eval; matches the rest of the suite — 40+ files already do this).HermesTaskClassifierso short keywords (ci,api,law) no longer match inside unrelated words. This fixes the real over-matching bug and makes the test deterministic. Multi-word keywords (stack trace,pull request) still match.Verification (local, Python 3.9.6 — the affected leg)
pytest tests/ -m "not integration and not requires_api and not requires_ollama and not requires_vllm"→ 1332 passed, 12 skipped, 59 deselected, 0 failed.test_high_stakes_domains_are_detectedpasses acrossPYTHONHASHSEED= 0/1/2/42/123 (was seed-dependent before).black==25.11.0 --check✓,ruff==0.15.0 check✓,mypy --ignore-missing-imports✓.Two focused commits; no behavior change beyond the classifier keyword-matching correctness.