fix: tie the trained wait to the trainer's own state - #189
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Beep boop! Here's the latest status of your PR checks. 🤖I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've checked the repo's reflexes (aka build speed). ⚡ ✅ All required files present. Latest Version: ✅ 🏷️ Release PreviewThe draft for the big day is ready for review. 📝 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)Ensuring our security logs are being captured. 🪵 ✅ No known vulnerabilities found (129 packages scanned). 🔍 LintHere's the latest update on this check. 🗞️ ❌ ruff: issues found — see job log ⚖️ License CheckEnsuring our license headers are up to date for 2024. 📅 ✅ No license violations found. Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 📊 CoverageCalculating the test-to-code ratio. ➗ Files below 80% coverage (15 files)
Full report: download the 🔨 Build TestsThe build process has successfully terminated. 🏁 ✅ All versions pass
The silent guardian of the dev branch. 🦇 |
48a90b7 to
512f8de
Compare
… say `mycroft.skills.trained` is emitted before the container it reports has finished compiling: padatious never trains on the calling thread, so the event fires while `needs_compile` is still true. A consumer that waits on the event and then matches an utterance races the compile, and loses under load. The wait now reads the trainer. A plugin offering `wait_until_trained` is asked directly, and `container.needs_compile` decides whether work is outstanding, so the deadline is pushed forward while a trainer still has work rather than expiring on a quiet bus. The wait is skipped when it cannot be needed: no skill registered an intent, or nothing on the bus trains one. The first of those is not merely an optimisation. A fresh container reports `needs_compile` before anything is registered in it, so a boot with no skills at all would otherwise wait for a compile nobody asked for -- measured at 4.08s against dev's 0.59s on this host, and the whole of ovoscope's own suite at 117s against dev's 113s once the skip is in place. `docs/minicroft.md` stated a 300-second pytest-timeout, which is below the wait's own worst case. That ceiling is `OVOSCOPE_TRAINED_MAX` plus `OVOSCOPE_TRAINED_TIMEOUT`, 780 seconds at the shipped defaults, and a framework timeout below it kills setup mid-wait and reads as a boot failure -- exactly the misdiagnosis that section exists to prevent. It now names the sum and asks for 900. Co-Authored-By: Claude Opus 5 (claude-opus-5) <noreply@anthropic.com>
512f8de to
c0a43ce
Compare
1 participant
This is the one durable fix for the trained wait, and it supersedes #178: it carries #178's
_DEFAULT_TRAINED_TIMEOUT = 180.0block verbatim, so #178 can close unmerged rather than the two competing.get_minicroftdecided whether to wait from two things it had happened to observe on the bus: the registrations seen so far, and whether anything subscribed tomycroft.skills.train. Both are the wrong instrument, and they fail in opposite directions.A boot that checked before the registrations landed saw an empty set, skipped the wait, and ran the suite against a padatious container that was still compiling — every utterance came back
ovos.intent.unmatched. That skip was traceless: its only log line sat insideif registered and not has_trainer, which cannot fire whenregisteredis empty. In the other direction the timeout was a wall clock on the whole training pass, so a skill with many locales on a loaded runner was declared stuck while padatious was still working, and theRuntimeErrorcame out ofsetUpClassand took the whole class with it.Training-requested is now a fact, not an observation.
load_plugin_skillsalways emitsmycroft.skills.trainafter loading, so it records that it did. Nothing has to be seen on the bus for the wait to be entered.Outstanding work is read from the trainers. A cleared
finished_training_eventsays a pass is running;needs_compileon a container says work exists but the background worker has not picked it up yet — that is the windowtrain()opens by design, since it never trains on the calling thread. A container the plugin has given up on is excluded, or the wait would never end. Where a plugin offerswait_until_trained, it is asked directly; it blocks on the container state the bus event only reports afterwards.Both guards survive. A bus subscriber that owns no training state still promises to report, and #179's loud raise still holds it to that. A load where no skill registered an intent still returns at READY.
Every skip says why, at INFO. Four distinct reasons, each named: opted out, nothing loaded, nothing trains, nothing registered.
The empty boot pays nothing. A fresh padatious container reports
needs_compilebefore anything is registered in it, so an earlier revision of this branch waited on a compile nobody had asked for: 4.08s for a boot with no skills at all, against 0.59s on dev, and ovoscope's own suite at 326s against 113s. The wait is now skipped whenever no skill registered an intent, whatever a container reports, and the same measurements are 0.58s and 117s.docs/minicroft.mdunderstated the ceiling. It asked for a 300-second pytest-timeout, below the wait's own worst case ofOVOSCOPE_TRAINED_MAXplusOVOSCOPE_TRAINED_TIMEOUT, which is 780 seconds at the shipped defaults. A framework timeout below that kills setup mid-wait and reads as a boot failure, which is the misdiagnosis that section exists to prevent. It now names the sum and asks for 900.OVOSCOPE_TRAINED_MAXoverallTests: 49 pass in
test_minicroft.py, including new cases for a dirty container before the pass starts, a container that only dirties after the wait is entered, a given-up container, and trainer discovery without any bus subscription. The full suite is 584 passed with 12 failures intest_listener_stream.py,test_golden.pyandtest_audit_round1.py— the same 12 fail on untoucheddevin the same environment, so they are inherited, not introduced.Two existing assertions changed, and it is worth saying why rather than burying it.
test_no_intents_registered_skips_waitasserted_trained_times == []andtest_trained_event_lets_quiet_window_elapse_and_returnasserted exactly one event. Both were really asserting that the boot returned before a trainer reported, which is a race and not a contract — a trainer with empty containers may report a pass anyway. They now assert the contract: the first that the boot does not spend the bound waiting, timed rather than counted; the second that a report was recorded and that no trainer is left holding work.Not yet demonstrated here: the acceptance runs.
ovos-skill-count#69's merged tree across five consecutive runs, and pokepedia fr-FR under load, both need this published as an alpha first, since the skills install ovoscope from PyPI. I will run them against the alpha and post the counts.