From b187a8d828d8130c67c4a0c247a02a994652c155 Mon Sep 17 00:00:00 2001 From: JarbasAi Date: Sun, 6 Sep 2026 14:08:51 +0100 Subject: [PATCH] fix: reach fallback-low tier and stop leaking state on a slow trainer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LEAN_DEFAULT_PIPELINE omitted ovos-fallback-pipeline-plugin-low, so any fallback skill registered above priority 90 (e.g. fallback-unknown at 100) was unreachable under ovoscope's default pipeline — the harness force-overrides both the default session pipeline and Configuration()["intents"]["pipeline"] to this same lean list, so the gap applied everywhere, not just to callers who opted into the lean default explicitly. Added the -low stage after -medium, in tier order. _DEFAULT_TRAINED_TIMEOUT defaulted to 5s outside CI. Any skill whose training took longer raised RuntimeError out of get_minicroft, which skips tearDownClass in E2E-style test classes built on it — leaking class-level monkeypatches and MiniCroft state into later, unrelated test files (observed corrupting six results in a downstream suite). Raised the default to 180s unconditionally instead of gating it behind the CI env var; the CI value was already sized for the worst observed case and a local machine is not guaranteed to be faster than a contended CI runner. Updated test_lean_default_excludes_heavy_pipelines, which asserted the absence of the -low stage as if it were a heavy/optional pipeline; replaced with an assertion that it is present, since it's a required tier not an opt-in extra. Self-tested: reachability (fallback-unknown fires and speaks under the default pipeline, confirmed to fail before the fix), non-regression (an Adapt-registered intent still matches via adapt-high, unaffected by the added tier), and the timeout default (a trained event arriving at 6s raises RuntimeError under the old 5s default, completes cleanly under the new 180s default). TestTrainedTimeoutDefaults now imports _DEFAULT_TRAINED_TIMEOUT from the module that defines it and asserts it equals 180.0. It previously recomputed a literal inline from os.environ.get("CI") and compared it to itself, so it stayed green for any value the module actually held. Mutating the constant to 5.0 turns three of its four tests red. Co-Authored-By: Claude Sonnet 5 --- docs/minicroft.md | 5 +-- ovoscope/__init__.py | 27 ++++++++------ test/unittests/test_minicroft.py | 64 +++++++++++++++++--------------- 3 files changed, 51 insertions(+), 45 deletions(-) diff --git a/docs/minicroft.md b/docs/minicroft.md index 18a15af..4c79462 100644 --- a/docs/minicroft.md +++ b/docs/minicroft.md @@ -134,8 +134,7 @@ and so does a boot whose pipeline has no such subscriber — for example `default_pipeline=M2V_PIPELINE` or an adapt-only pipeline — since nothing will ever report training done. If an intent was registered, a subscriber is present, and training never completes within `OVOSCOPE_TRAINED_TIMEOUT` -seconds (default: 180s when the `CI` environment variable is set, 5s -otherwise), `get_minicroft()` raises `RuntimeError` naming only the +seconds (default: 180s), `get_minicroft()` raises `RuntimeError` naming only the skill(s) that registered an intent and never got a `mycroft.skills.trained` reply — a stuck trainer in one skill never blames an unrelated, intentless skill loaded alongside it. Pass `wait_for_trained=False` to opt out. @@ -183,7 +182,7 @@ croft = get_minicroft( ) ``` -When testing with N secondary languages, training overhead scales with the number of per-language containers — for example, a 17-locale suite may require 129 seconds for unconstrained training (on a system without resource limits). `max_wait` bounds only the wait for `READY`; it has no effect on the training wait that follows. The training wait is bounded by `OVOSCOPE_TRAINED_TIMEOUT`, and its default (180s under `CI`, 5s otherwise) is tuned for single-language loads. For multilingual suites, set `OVOSCOPE_TRAINED_TIMEOUT` large enough to accommodate all language engines, and raise `max_wait` too if reaching `READY` itself is slow with that many engines starting up: +When testing with N secondary languages, training overhead scales with the number of per-language containers — for example, a 17-locale suite may require 129 seconds for unconstrained training (on a system without resource limits). `max_wait` bounds only the wait for `READY`; it has no effect on the training wait that follows. The training wait is bounded by `OVOSCOPE_TRAINED_TIMEOUT`, and its default of 180s is tuned for single-language loads. For multilingual suites, set `OVOSCOPE_TRAINED_TIMEOUT` large enough to accommodate all language engines, and raise `max_wait` too if reaching `READY` itself is slow with that many engines starting up: ```python import os diff --git a/ovoscope/__init__.py b/ovoscope/__init__.py index 6ed7506..e11ebf8 100644 --- a/ovoscope/__init__.py +++ b/ovoscope/__init__.py @@ -182,6 +182,7 @@ "ovos-padacioso-pipeline-plugin-medium", "ovos-fallback-pipeline-plugin-high", "ovos-fallback-pipeline-plugin-medium", + "ovos-fallback-pipeline-plugin-low", ] # Standard test pipeline — all standard built-in stages. @@ -1182,18 +1183,20 @@ def _restore_default_session(self): # for). TRAINED_QUIET_WINDOW = 0.5 -# The overall bound on the trained-wait is env-tunable so CI (slower, cold -# caches, contended runners) gets a generous default while local runs stay -# tight. Presence of the CI env var (not its value) selects the default. -# CI default is 180s: worst-case uninstrumented on taskset-2 was 16.8s, but -# fleet CI jobs run under coverage instrumentation on throttled 2-core shared -# VMs where a large single-skill intent set exceeded 60s in the field (weather: -# 262 trained-timeout failures at 60s; the alerts multilang fixture -# independently documents "under coverage instrumentation, booting reliably -# needs more than 60s"). 180s serves the real condition, costs nothing on -# healthy boots (quiet-window return), and the loud never-trained guard still -# fires. -_DEFAULT_TRAINED_TIMEOUT = 180.0 if os.environ.get("CI") else 5.0 +# The overall bound on the trained-wait is env-tunable, but the default +# itself must be generous everywhere, not just under a CI env var: a 5s +# local default is shorter than plenty of real skills' training time, and a +# timeout here doesn't just fail the current test — it raises out of +# setUpClass, skipping tearDownClass, which leaves class-level monkeypatches +# and MiniCroft state leaked into later, unrelated test files. 180s: worst-case +# uninstrumented on taskset-2 was 16.8s, but fleet CI jobs run under coverage +# instrumentation on throttled 2-core shared VMs where a large single-skill +# intent set exceeded 60s in the field (weather: 262 trained-timeout failures +# at 60s; the alerts multilang fixture independently documents "under coverage +# instrumentation, booting reliably needs more than 60s"). 180s serves the +# real condition, costs nothing on healthy boots (quiet-window return), and +# the loud never-trained guard still fires. +_DEFAULT_TRAINED_TIMEOUT = 180.0 def get_minicroft(skill_ids: Union[List[str], str], *args, diff --git a/test/unittests/test_minicroft.py b/test/unittests/test_minicroft.py index e35c7d0..0891207 100644 --- a/test/unittests/test_minicroft.py +++ b/test/unittests/test_minicroft.py @@ -13,7 +13,8 @@ from ovoscope import (MiniCroft, get_minicroft, DEFAULT_TEST_PIPELINE, LIGHT_TEST_PIPELINE, ADAPT_PIPELINE, LEAN_DEFAULT_PIPELINE, - M2V_PIPELINE, PERSONA_PIPELINE, is_pipeline_available) + M2V_PIPELINE, PERSONA_PIPELINE, is_pipeline_available, + _DEFAULT_TRAINED_TIMEOUT) LEGACY_UTTERANCE = "recognizer_loop:utterance" SPEC_UTTERANCE = str(SpecMessage.UTTERANCE) # ovos.utterance.handle @@ -696,13 +697,20 @@ def tearDown(self): LOG.set_level("CRITICAL") def test_lean_default_excludes_heavy_pipelines(self): - """LEAN_DEFAULT_PIPELINE must not reference m2v/persona/common_query/OCP.""" + """LEAN_DEFAULT_PIPELINE must not reference m2v/persona/common_query/OCP. + + The fallback -low tier is intentionally included: fallback skills + registered above priority 90 (e.g. fallback-unknown at 100) are only + reachable through ovos-fallback-pipeline-plugin-low, and it is not a + "heavy" pipeline plugin in the sense this test guards against. + """ for stage in LEAN_DEFAULT_PIPELINE: self.assertNotIn("m2v", stage, f"m2v stage found: {stage}") self.assertNotIn("persona", stage, f"persona stage found: {stage}") self.assertNotIn("common-query", stage, f"common_query stage found: {stage}") self.assertNotIn("ocp", stage, f"OCP stage found: {stage}") - self.assertNotIn("-low", stage, f"-low tier stage found: {stage}") + self.assertIn("ovos-fallback-pipeline-plugin-low", LEAN_DEFAULT_PIPELINE, + "fallback-low tier must be reachable in the lean default") def test_lean_default_boots_only_lean_plugins(self): """A lean-default MiniCroft must not instantiate heavy pipeline @@ -767,41 +775,37 @@ def test_bogus_pipeline_id_raises_naming_it(self): class TestTrainedTimeoutDefaults(unittest.TestCase): - """Verify that the OVOSCOPE_TRAINED_TIMEOUT default is 60s in CI and 5s locally. - - This is a regression test ensuring the timeout scales appropriately: CI - (slower, cold caches) gets a generous default, while local runs stay tight. - """ + """Guard the trained-wait default and the environment override.""" def setUp(self): LOG.set_level("ERROR") - import os as os_module - self.os_module = os_module def tearDown(self): LOG.set_level("CRITICAL") - def test_ci_default_timeout_is_180_seconds(self): - """When CI=true, the default computed timeout must be 180s.""" - # Test the logic: when CI env var is present, default should be 180s - with patch.dict("os.environ", {"CI": "true"}): - timeout = 180.0 if self.os_module.environ.get("CI") else 5.0 - self.assertEqual(timeout, 180.0, - "CI default timeout must be 180s to accommodate cold caches, " - "coverage instrumentation, and contended runners") - - def test_local_default_timeout_is_5_seconds(self): - """When CI is not set, the default computed timeout must be 5s.""" - # Test the logic: when CI is absent, default should be 5s + def test_default_timeout_is_180_seconds(self): + """The trained-wait default is 180s, with no CI distinction.""" + self.assertEqual(_DEFAULT_TRAINED_TIMEOUT, 180.0, + "Trained-wait default must be 180s to accommodate cold " + "caches, coverage instrumentation, and contended runners") + + def test_default_timeout_does_not_depend_on_ci(self): + """Clearing or setting CI does not change the default.""" with patch.dict("os.environ", {}, clear=True): - timeout = 60.0 if self.os_module.environ.get("CI") else 5.0 - self.assertEqual(timeout, 5.0, - "Local default timeout must be 5s for fast iteration") + self.assertEqual(_DEFAULT_TRAINED_TIMEOUT, 180.0) + with patch.dict("os.environ", {"CI": "true"}): + self.assertEqual(_DEFAULT_TRAINED_TIMEOUT, 180.0) def test_ovoscope_trained_timeout_honors_env_var(self): - """The OVOSCOPE_TRAINED_TIMEOUT env var is honored over the computed default.""" + """The OVOSCOPE_TRAINED_TIMEOUT env var wins over the default.""" with patch.dict("os.environ", {"OVOSCOPE_TRAINED_TIMEOUT": "120"}): - timeout_str = self.os_module.environ.get("OVOSCOPE_TRAINED_TIMEOUT") - timeout = float(timeout_str) if timeout_str else None - self.assertEqual(timeout, 120.0, - "OVOSCOPE_TRAINED_TIMEOUT env var should be respected") + timeout = float(os.environ.get("OVOSCOPE_TRAINED_TIMEOUT", + _DEFAULT_TRAINED_TIMEOUT)) + self.assertEqual(timeout, 120.0) + + def test_default_used_when_env_var_absent(self): + """Without the env var the module default is what gets read.""" + with patch.dict("os.environ", {}, clear=True): + timeout = float(os.environ.get("OVOSCOPE_TRAINED_TIMEOUT", + _DEFAULT_TRAINED_TIMEOUT)) + self.assertEqual(timeout, 180.0)