Skip to content

refactor DAP _do_fetch() method#5213

Open
huangjeff5 wants to merge 22 commits intomainfrom
jh-dap-pr3
Open

refactor DAP _do_fetch() method#5213
huangjeff5 wants to merge 22 commits intomainfrom
jh-dap-pr3

Conversation

@huangjeff5
Copy link
Copy Markdown
Contributor

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.)

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Base automatically changed from jh-dap-pr2 to main April 30, 2026 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant