-
Notifications
You must be signed in to change notification settings - Fork 56
fix(engine): warn when the provider pricing hook returns nothing for every model #388
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
69add20
a2669c7
245c9d8
99dcb4a
a29d1c9
3aa1335
7c6c6d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -428,6 +428,16 @@ def __init__( | |
| # at debug level — otherwise table-priced models still show a normal | ||
| # cost and the broken live pricing is invisible (see #265). | ||
| self._pricing_hook_failed_warned = False | ||
| # The companion case: a hook that never raises but returns ``None`` for | ||
| # everything, which is indistinguishable from "these models are simply | ||
| # unpriced" unless it is tracked. Live today on Copilot: the SDK's | ||
| # hand-written ``client.ModelBilling`` parses only ``multiplier`` and | ||
| # discards the ``tokenPrices`` wire field, so the hook resolves ``None`` | ||
| # for every model. Verified against the pinned 1.0.1; 1.0.9 parses the | ||
| # field, so a version bump may be the real fix. See #386. | ||
| self._pricing_hook_none_models: set[str] = set() | ||
| self._pricing_hook_priced_any = False | ||
| self._pricing_hook_silent_warned = False | ||
|
|
||
| # One-time latch so the "budget set but no pricing" degraded warning | ||
| # is emitted at most once per workflow run (see _check_budget). | ||
|
|
@@ -2059,6 +2069,76 @@ async def _get_provider_for_agent(self, agent: AgentDef) -> AgentProvider | None | |
| return None | ||
| return self._single_provider | ||
|
|
||
| def _note_pricing_hook_result(self, model: str, pricing: ModelPricing | None) -> None: | ||
| """Record what the provider pricing hook returned for ``model``. | ||
|
|
||
| Pure bookkeeping — deliberately no verdict here. Whether the hook is | ||
| systemically silent is only answerable once the run has finished asking | ||
| it: a hook that declines A and B and then prices C is working fine, and | ||
| deciding on the second ``None`` would already have warned. Because | ||
| :meth:`_ensure_pricing_resolved` locks per model rather than globally, | ||
| arrival order varies under parallel and ``for_each`` groups, so an early | ||
| verdict is also nondeterministic — the same workflow warns on one run | ||
| and stays quiet on the next. | ||
|
|
||
| The conclusion is drawn once in :meth:`_warn_if_pricing_hook_silent`. | ||
|
|
||
| Args: | ||
| model: The model whose pricing was just resolved. | ||
| pricing: The hook's result — ``None`` when it declined to price. | ||
| """ | ||
| if pricing is not None: | ||
| self._pricing_hook_priced_any = True | ||
| return | ||
|
|
||
| self._pricing_hook_none_models.add(model) | ||
|
|
||
| def _warn_if_pricing_hook_silent(self) -> None: | ||
| """Conclude, once per run, whether live pricing was ever available. | ||
|
|
||
| A hook returning ``None`` for a single model is ordinary — that model | ||
| simply is not priced, and the static table covers it. A hook that | ||
| returned ``None`` for *every* model it was asked about, having priced | ||
| nothing, is a different condition: the mechanism is not working, and | ||
| every cost in the run came from the static fallback table. | ||
|
|
||
| No minimum-model floor. Running to completion having priced nothing is | ||
| the evidence; a single-model workflow is the common case (most shipped | ||
| examples resolve exactly one model, and #386's own reproduction is a | ||
| single-model run), so a floor of two would exempt precisely the runs | ||
| most likely to hit this. | ||
| """ | ||
| if ( | ||
| self._pricing_hook_silent_warned | ||
| or self._pricing_hook_priced_any | ||
| or not self._pricing_hook_none_models | ||
| ): | ||
| return | ||
|
|
||
| self._pricing_hook_silent_warned = True | ||
| models = ", ".join(sorted(self._pricing_hook_none_models)) | ||
| count = len(self._pricing_hook_none_models) | ||
| logger.warning( | ||
| "Provider pricing hook returned no pricing for any of the %d models " | ||
| "resolved so far (%s). Costs for models in the static pricing table " | ||
| "are estimates from that table; models missing from it are reported " | ||
| "as unpriced. Set `cost.pricing` in the workflow to supply rates.", | ||
| count, | ||
| models, | ||
| ) | ||
| # Conductor installs no logging handlers, so the line above reaches | ||
| # ``logging.lastResort`` as unattributed stderr — absent from the JSONL | ||
| # log and the dashboard, and under ``--web-bg`` written to a temp file | ||
| # nobody was told to read. Emit it as an event too, the same shape | ||
| # ``checkpoint_save_failed`` uses. | ||
| self._emit( | ||
| "pricing_hook_silent", | ||
| { | ||
| "models": sorted(self._pricing_hook_none_models), | ||
| "model_count": count, | ||
| }, | ||
| ) | ||
|
|
||
| async def _ensure_pricing_resolved(self, agent: AgentDef, model: str | None) -> None: | ||
| """Resolve provider-supplied pricing for ``model`` and cache it. | ||
|
|
||
|
|
@@ -2129,6 +2209,22 @@ async def _ensure_pricing_resolved(self, agent: AgentDef, model: str | None) -> | |
| ) | ||
| else: | ||
| self.usage_tracker.set_provider_pricing(model, pricing) | ||
| # Only track providers that actually implement the hook. | ||
| # ``AgentProvider.get_model_pricing`` returns ``None`` by | ||
| # design and ``providers/base.py`` documents that as the | ||
| # correct behaviour for a provider whose SDK exposes no | ||
| # pricing; only Copilot overrides it. Without this check, | ||
| # four of the five providers get told their SDK broke for | ||
| # doing exactly what the base class prescribes. | ||
| # | ||
| # Imported here rather than at module scope: the | ||
| # top-level import is ``TYPE_CHECKING``-only. | ||
| from conductor.providers.base import ( | ||
| AgentProvider as _AgentProviderBase, | ||
| ) | ||
|
|
||
| if type(provider).get_model_pricing is not _AgentProviderBase.get_model_pricing: | ||
| self._note_pricing_hook_result(model, pricing) | ||
| # Mark resolved only now — after the attempt completes — even on a | ||
| # provider-None / failure / None result, so it isn't retried on every | ||
| # record and the model stays unpriced via the static-table fallback. | ||
|
|
@@ -2211,7 +2307,15 @@ async def run(self, inputs: dict[str, Any]) -> dict[str, Any]: | |
| # Execute on_start hook | ||
| self._execute_hook("on_start") | ||
|
|
||
| result = await self._execute_loop(current_agent_name) | ||
| try: | ||
| result = await self._execute_loop(current_agent_name) | ||
| finally: | ||
| # The pricing verdict belongs to the run ending, not to anyone | ||
| # asking for a summary. Drawing it here covers the run that dies | ||
| # part way -- the case where "these numbers came from the static | ||
| # table" matters most, and the one a summary-time call can never | ||
| # reach, because the CLI re-raises before it asks. | ||
| self._warn_if_pricing_hook_silent() | ||
| # Successful completion: this run's periodic checkpoints are now stale. | ||
| self._cleanup_run_periodic_checkpoints() | ||
| return result | ||
|
|
@@ -2248,7 +2352,12 @@ async def resume(self, current_agent_name: str) -> dict[str, Any]: | |
| # Execute on_start hook (signals resume) | ||
| self._execute_hook("on_start") | ||
|
|
||
| result = await self._execute_loop(current_agent_name) | ||
| try: | ||
| result = await self._execute_loop(current_agent_name) | ||
| finally: | ||
| # Same reasoning as :meth:`run` -- a resumed run that dies part way | ||
| # is still a run that priced nothing. | ||
| self._warn_if_pricing_hook_silent() | ||
| # Successful completion: this run's periodic checkpoints are now stale. | ||
| self._cleanup_run_periodic_checkpoints() | ||
| return result | ||
|
|
@@ -6798,12 +6907,22 @@ def get_execution_summary(self) -> dict[str, Any]: | |
| summary["parallel_agents_count"] = parallel_agents_count | ||
|
|
||
| # Add usage/cost information | ||
| # Normally already drawn by :meth:`run` / :meth:`resume` when the run | ||
| # ended; the call is idempotent. It stays here for callers that drive | ||
| # the engine without those entry points, so the flag below is never | ||
| # reported as ``False`` merely because nothing concluded the run. | ||
| self._warn_if_pricing_hook_silent() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the only caller of the verdict, and it sits past the point where a failing run has already re-raised. I ran the three cases on this branch with a hook that returns The failing run is the one I care about most. That's when someone is squinting at cost numbers trying to work out what they spent before it died.
|
||
| usage = self.usage_tracker.get_summary() | ||
| summary["usage"] = { | ||
| "total_input_tokens": usage.total_input_tokens, | ||
| "total_output_tokens": usage.total_output_tokens, | ||
| "total_tokens": usage.total_tokens, | ||
| "total_cost_usd": usage.total_cost_usd, | ||
| # A model priced from the static table has a ``cost_usd`` and so | ||
| # never appears in ``unpriced_models``. Without this flag the | ||
| # summary prints a confident number sourced from a possibly stale | ||
| # table while the explanation goes only to stderr. | ||
| "live_pricing_degraded": self._pricing_hook_silent_warned, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing consumes this. The event a few lines up has a softer version of the same problem. It reaches the JSONL log fine, since Worth trimming the CHANGELOG line to match whatever you decide here. As it stands it claims the dashboard, and that isn't wired. |
||
| "unpriced_agent_count": len(usage.unpriced_agents), | ||
| "unpriced_models": usage.unpriced_models, | ||
| "agents": [ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gate is correct. I checked it against the real classes rather than a stand-in:
ClaudeProvider,HermesProvider,AcaRuntimeProviderandClaudeAgentSdkProviderall inherit the base and get skipped, and Copilot is the only one tracked. That closes the false positive properly.It just isn't covered. I swapped this whole condition for an unconditional call and both pricing test files stayed green at 16 passed.
TestBaseHookProvidersAreNotAccusedreads like the test for it, but what it actually asserts is_Inherits.get_model_pricing is AgentProvider.get_model_pricingon two throwaway subclasses. That's Python attribute lookup, which will hold whether or not this line exists.