docs: note on non-transparent wrapper evaluators under GraphEvaluator - #260
Merged
Conversation
Add a subsection to the "Write a custom evaluator" how-to explaining that a result-reshaping or publishing wrapper evaluator must gate on context.fn == "__call__". Because GraphEvaluator re-drives __deps__ and child nodes through the same evaluator stack, a wrapper that fires on every invocation crashes on the GraphDepList payload of __deps__ and side-effects child results (double-publish, cache-identity mismatch). Documents that scoping the override to specific models/model_types stops firing on dependencies but is not sufficient on its own, since a model still re-drives its own __deps__, and points at DryRunEvaluator as a worked example. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Pascal Tomecek <pascal.tomecek@cubistsystematic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
=======================================
Coverage 93.48% 93.48%
=======================================
Files 176 176
Lines 20327 20327
Branches 1350 1350
=======================================
Hits 19002 19002
Misses 1052 1052
Partials 273 273 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
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
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.
Summary
Adds a docs subsection to the "Write a custom evaluator" how-to (
docs/wiki/how-to/Cache-Results.md) covering a subtle footgun when a non-transparent wrapper evaluator (one that reshapes, replaces, or publishes the result) is combined withGraphEvaluator.GraphEvaluatorre-drives both__deps__resolution and child-node evaluations through the same evaluator stack. A wrapper that fires on every invocation therefore:GraphDepListpayload of__deps__(not a model result) — reshaping logic such as.dfaccess crashes there; andWhat the note says
context.fn == "__call__"so it never acts on__deps__(includes a smallPublishingEvaluatorexample).models/model_typesto avoid firing on dependencies — but note this is not sufficient on its own, because a model still re-drives its own__deps__, so thefn == "__call__"guard is always required.DryRunEvaluatoras a worked example (passes through on__deps__, uses a re-entry guard for child drives).The findings were verified empirically against a
GraphEvaluatorstack.Notes
mdformat --checkandcodespellpass.