refactor DAP _do_fetch() method#5213
Conversation
…enerate integration
There was a problem hiding this comment.
Code Review
This pull request refactors the Dynamic Action Provider (DAP) to support tracing by executing fetches through an action and introduces a set_value method for cache management. Additionally, the registry's DAP resolution logic was updated to attempt running the DAP action instead of immediately raising a RuntimeError. Feedback was provided regarding the error handling in _do_fetch, specifically that unwrapping GenkitError via e.cause may lead to the loss of relevant error context and metadata.
| try: | ||
| await self.action.run() | ||
| except GenkitError as e: | ||
| raise e.__cause__ or e |
There was a problem hiding this comment.
The logic raise e.__cause__ or e might inadvertently unwrap a user-defined GenkitError if it was raised with a cause (e.g., raise GenkitError(..., cause=err) from err). In such cases, e.__cause__ would be the underlying error, and the user's GenkitError message and status would be lost. Since Action.run is known to wrap non-GenkitError exceptions with a specific wrapper message, it might be safer to only unwrap if the error is indeed that wrapper, or simply propagate the GenkitError as is, as it is the standard exception type for the framework.
Before: We called the lazy fn() first, stored the result on the provider, then called action.run() with essentially no work inside. The span did not cover the time spent in fn(). This was a subtle deviation from the expected behavior.
After: We call action.run() first. Its body runs fn() and then set_value() on the provider. The trace span covers the real fetch. For reflection listing we use skip_trace and still run fn() + set_value() directly so we do not spam traces.
Why: Traces should show one meaningful “DAP ran” interval that matches actual resolution work, not a hollow step after the fact.
(In JS, getOrFetch either calls dap.run() (whose body calls fn() and updates the cache) or, when skipping trace, calls fn() and setValue without action.run. We mirror that behavior here.)