feat(probes): signal when an IntentProbe is skipped for inactive intents#1903
feat(probes): signal when an IntentProbe is skipped for inactive intents#1903WatchTree-19 wants to merge 1 commit into
Conversation
leondz
left a comment
There was a problem hiding this comment.
fine catch, this situation should be handled appropriately
there is a more general case that could probably be dealt with here. for any probe that fails to produce prompts, the probe behaviour should lead to the harness skipping (via "SKIPPED") that probe. could be mediated via exception or via probe return.
suggestion:
- add a hook method in probes.base.Probe for checking probe consistency (bool)
- update harness to call this method between probe instantiation and calling of probe()
- in the probe's method for handling the error, add a check for consistency and return value, e.g.
if not self.prompts:
return False
return True
- add a test for a probe having no prompts that ends up being skipped by harness
- be careful for probes that intentionally don't know all their prompts before probe() is called
- add a test that checks probes for consistency after loading them
- check that tests pass
alternatively another hook location might work, or passing by exception might work, let's see
| logging.debug("%s has no active intents; no prompts to send", self.probename) | ||
| # yields no prompts; no-op so the rest of the run proceeds (3A). Surface | ||
| # the skip so it is not silent on the terminal or in the report (#1889). | ||
| self._notify_skipped_no_active_intents() |
There was a problem hiding this comment.
why have this method here?
- seems like there should be some logging etc whenever self.prompts is empy
- a guard also belongs around L385 in Probe
- the behavior should be to SKIP at harness level
- value of this method is limited with such tightly scoped functionality
- probes don't write to stdout
- concrete probes don't write to reports
| f"no prompts to send (check your intent: run.spec selector)" | ||
| ) | ||
| logging.info(msg) | ||
| print(f"⚠️ {msg}") |
There was a problem hiding this comment.
no stdout from probes thanks
| ) | ||
| logging.info(msg) | ||
| print(f"⚠️ {msg}") | ||
| reportfile = getattr(_config.transient, "reportfile", None) |
There was a problem hiding this comment.
_config shouldn't be externally accessed from probes during runtime
| print(f"⚠️ {msg}") | ||
| reportfile = getattr(_config.transient, "reportfile", None) | ||
| if reportfile is not None: | ||
| reportfile.write( |
There was a problem hiding this comment.
probe's should not write to report about things other than attempts
Address review on NVIDIA#1889: instead of a bespoke IntentProbe method that printed and wrote to the report, handle the general case. - base Probe.probe: guard empty self.prompts (logging.info + return []), which also fixes a latent prompts[0] IndexError. - remove the IntentProbe.probe override; base now covers it. - harness: skip and log when a probe returns no attempts. - no stdout from probes, no _config access, no report writes. Update tests to assert the logging / no-stdout / no-report behaviour. Signed-off-by: WatchTree-19 <119982314+WatchTree-19@users.noreply.github.com>
87efd07 to
972f460
Compare
|
thanks, that all makes sense. reworked:
one judgment call: i keyed the harness skip off an empty result rather than tests updated to cover the logging / no-stdout / no-report behaviour. |
Description
An
IntentProbewhose intents are all outside the active set (e.g. filtered out by anintent:run.spec selector) produces no prompts and returns early fromprobe(). Before this change that path emitted only alogging.debug, so the skip was silent: the terminal showed no notice and the report carried no context for the missing probe (#1889).This adds
_notify_skipped_no_active_intents(), called on that early return, which:⚠️ probe ... skipped: none of its intents are in the active set ...), matching the existingwarn_unconsumed_intentsconvention, andprobe_skippedreport entry (reason: no_active_intents, plus the intents considered) so the skip surfaces in the report/digest.Targets
feature/technique_intentsince the intent run-spec selector (from #1866) lives on that branch, notmain.Fixes #1889.
Type of change
Verification
Added
tests/cas/test_intent_skip_notice.py, which verifies the skipped probe (a) writes exactly oneprobe_skippedreport entry withreason: no_active_intents, and (b) prints the terminal notice. Existingtests/cas/test_cas_intentprobe.pyandtests/cas/test_intent_run_spec.pystill pass (20 tests).ruff/blackclean on the changed lines.