Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #231 +/- ##
==========================================
+ Coverage 93.09% 93.31% +0.21%
==========================================
Files 169 175 +6
Lines 18748 19850 +1102
Branches 1235 1276 +41
==========================================
+ Hits 17454 18523 +1069
- Misses 1066 1089 +23
- Partials 228 238 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Introduce a structured reporting layer that captures evaluation metadata,
timing, topology, retries, and failures without consuming result payloads,
mirroring the existing evaluator/policy architecture.
- ReportingPolicy shared core with span/run ContextVars for nested,
thread/async-local isolation
- ReportEvent model plus NoOp/InMemory/Logging/Composite/UI reporters and
a bounded UI polling buffer
- Tracing/metrics/alerts policies and OpenTelemetry tracing/metrics
integration (exposed via otel/full/develop/test extras)
- Structural reporting models and a <Vendor><Signal>Reporting{Evaluator,Model}
taxonomy with placeholder vendor classes
- Refactor LoggingEvaluator onto LoggingPolicy to share formatting and
enable LoggingModel while preserving the existing import path and log output
- Retry lifecycle events now carry run_id and child depth via
current_span_depth(); reporter failures are isolated on reporting/retry paths
- DryRunEvaluator with context-local planning guard; synthetic mode is
non-transparent so results are not cached under real-run keys; node_key
strips the dry-run evaluator layer while preserving non-evaluator options
so it matches cache_key() for the logical node
- ReportingStateStore preserves terminal outcomes while allowing retry streams
to progress
- Docs: reporting workflow, reporter options, OpenTelemetry install, reserved
run/graph phases, extra payload keys, and dry-run synthetic-result warnings
- Tests across utils/evaluators/models covering success/error flows, dry-run
override recursion, cache composition, concurrent dry-run reuse, node-key
semantics, retry event nesting, reporter failure isolation, and state folding
Signed-off-by: Tim Paine <3105306+timkpaine@users.noreply.github.com>
ptomecek
left a comment
ptomecek
left a comment
There was a problem hiding this comment.
Took a pass through this. I really like how the reporting policy/evaluator/model split mirrors the retry trio (RetryPolicy/RetryEvaluator/RetryModel) - it slots in cleanly and reads exactly the way I'd expect. A few things I'd like to sort out before it goes in, left inline.
One non-code note: the description has a "Docs: reporting workflow, reporter options, ..." bullet, but I don't see any docs/ changes in the diff. Did those get dropped, or is the description just ahead of the code? Given how much public API this adds, I think we'll want the docs to land with it.
| return self._run_with_reporting(context, **_descriptor(context)) | ||
|
|
||
|
|
||
| class DryRunEvaluator(EvaluatorBase, ReportingPolicy): |
There was a problem hiding this comment.
DryRunEvaluator feels a bit out of place here. It's really a planning/preview tool rather than telemetry, but it's sitting in reporting.py and inheriting ReportingPolicy - I don't think anyone hunting for "dry run" would think to look under reporting, and inheriting the policy means it drags reporting config along even when you just want the plan.
It does emit QUEUED/SKIPPED so the dependency on reporting is real, but could we move it to common.py (or its own module) and compose reporting instead of inheriting it? At a minimum I'd like a note explaining why it lives here, since the placement isn't obvious.
|
|
||
|
|
||
| # ***************************************************************************** | ||
| # Vendor placeholders (not yet implemented) |
There was a problem hiding this comment.
Can we hold off on shipping these? Right now there are 9 classes that only raise NotImplementedError, and they're all in all - that's more dead names than working ones in this module's public surface, and we don't ship stub classes like this anywhere else in ccflow.
I'd rather add each one when its backend actually exists. If the point is just to stake out the taxonomy, a comment or a private list is a lot lighter than 9 exported classes we'd then have to keep around and eventually deprecate if the plan changes. Config can still reference an unimplemented path via PyObjectPath without a shipped class.
|
|
||
| # -- lifecycle hooks (override in subclasses) ---------------------------- | ||
|
|
||
| def _on_start(self, ctx: ReportContext, span: "Optional[Span]") -> None: |
There was a problem hiding this comment.
Since LoggingEvaluator is the default evaluator, this hook runs on basically every model evaluation - and _event() builds a full ReportEvent here even when reporter is None, which is the common case. _emit then just discards it. I measured it at roughly 11us/eval across the three events (START/SUCCESS/END), all wasted when nobody's reporting.
Could we short-circuit when self.reporter is None before constructing the event? The inline logging in LoggingPolicy still needs to fire, but the base emit hooks shouldn't have to build anything if there's no sink.
2 participants
Introduce a structured reporting layer that captures evaluation metadata, timing, topology, retries, and failures without consuming result payloads, mirroring the existing evaluator/policy architecture.