Add display banner#5754
Conversation
github-actions
Bot
added
size: M
|
I'll merge #5752 and then update here to fix CI |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the banner feature and the temporal passthrough additions. The passthrough list and the new regression test in tests/test_temporal.py look solid — good comment chain explaining why each module is added and a canary case for OpenAI to catch future SDK changes.
The banner feature itself is well-factored (lazy import, helper-per-field, single private module), but it has a few issues worth addressing before this lands. They are all in _display.py and agent/__init__.py; see inline comments for details. Top two are:
id(agent)-based dedup in_display.pycollides after GC and grows without bound — use aWeakSet[Agent]or attach a monotonic counter to the instance.- The 152-line new module ships with zero test coverage, which violates the project's "100% of code paths" testing guideline.
Also worth a quick follow-up: the new PYDANTIC_AI_HIDE_BANNER env var is the only opt-out and is not documented anywhere except inside the banner it suppresses.
Approving so the passthrough fixes aren't blocked, but the inline findings are real.
| ot = agent.output_type | ||
| return getattr(ot, '__name__', str(ot)) | ||
| except Exception: | ||
| return 'str' |
There was a problem hiding this comment.
LOW — Fallback output_type: 'str' is misleading
The exception path returns the literal string 'str'. If agent.output_type ever raises (e.g. a property error on a user subclass), the banner tells the user the agent outputs a str, which is likely not true — it could be a structured Pydantic model, an int, a custom class, anything. The fallback should be neutral ('(unknown)', `'<?>') so the displayed value is never silently wrong. A real failure here should also be loud enough to investigate, not hidden behind a plausible-looking default.
Docs Preview
|
There was a problem hiding this comment.
Banner feature is well-factored (lazy import, helper-per-field, single private module) and the new test coverage is solid. The prior review's three inline findings (id(agent) dedup, 'str' fallback, unused timestamp) are all cleanly resolved, and the process-wide boolean dedup is the right simplification.
One new issue worth fixing before merge: _display._get_logfire_status uses agent.instrument (the per-instance field) instead of agent._resolve_instrumentation_settings() (which also consults the Agent._instrument_default set by Agent.instrument_all(...)). This is a regression from the prior commit and the banner will say "logfire: not configured" for users who configured logfire via instrument_all — see inline comment on _display.py:111 for the trigger and a suggested fix.
Two minor stale docstring/comment issues are also flagged inline; both are one-line fixes.
Verdict: APPROVE — no blocking issues.
| if infer_name and self.name is None: | ||
| self._infer_name(inspect.currentframe()) | ||
|
|
||
| # Display agent banner on first run (once per agent instance, process-wide) |
There was a problem hiding this comment.
LOW — Stale comment: "once per agent instance, process-wide"
Same wording bug as the display_banner docstring above. The implementation is once per process (single boolean _banner_displayed), and the phrase is internally inconsistent. Update to something like # Display agent banner on first run (once per process).
| def _get_logfire_status(agent: _Agent[Any, Any]) -> str: | ||
| """Check whether Logfire instrumentation is configured.""" | ||
| try: | ||
| return 'configured' if agent.instrument else 'not configured' |
There was a problem hiding this comment.
MEDIUM — Banner mis-reports logfire status when Agent.instrument_all(True) is set
agent.instrument is the public property that returns the raw _instrument field (defaults to None per-instance, see agent/__init__.py:798-801 and :424 where self._instrument = None). The global default set by Agent.instrument_all(...) lives on the class-level Agent._instrument_default and is only consulted by _resolve_instrumentation_settings() (line 2386) — not by agent.instrument.
Concrete trigger:
from pydantic_ai import Agent
Agent.instrument_all(True)
agent = Agent('openai:gpt-5-mini', defer_model_check=True)
agent.display_banner() # prints "logfire: not configured" — wrongA user who follows the "instrument this agent" step in the banner's own next-steps list will see the banner say "not configured" even though logfire is wired up for their agent. This is a regression introduced in the "display updates" commit — the prior version (commit 3abade34) used agent._resolve_instrumentation_settings() for exactly this reason:
return 'configured' if settings is not None else 'not configured'| return 'configured' if agent.instrument else 'not configured' | |
| try: | |
| settings = agent._resolve_instrumentation_settings() | |
| return 'configured' if settings is not None else 'not configured' | |
| except Exception: | |
| return 'unknown' |
Also worth adding a test that pins this behavior — e.g. test_logfire_status_configured_via_instrument_all that sets Agent.instrument_all(True), constructs an agent, and asserts 'configured' in output.
|
|
||
| def test_hide_banner_env_var(self, monkeypatch: pytest.MonkeyPatch): | ||
| """PYDANTIC_AI_HIDE_BANNER suppresses output.""" | ||
| _display._banner_displayed = False |
There was a problem hiding this comment.
NITPICK — Redundant manual reset of _display._banner_displayed
The autouse _reset_banner_state fixture at lines 12-17 already sets _display._banner_displayed = False both before and after every test in this module, so this line is dead code. Drop it (or, if you want to keep the explicit intent, drop the autouse fixture — but the fixture's name and docstring make the "reset between tests" invariant clearer).
| def display_banner(self) -> None: | ||
| """Display a banner with logo, agent info, and next steps. | ||
|
|
||
| Prints to stderr once per agent instance. Subsequent calls are no-ops. |
There was a problem hiding this comment.
LOW — Stale docstring says "once per agent instance"
The actual behavior is once per process (driven by _display._banner_displayed: bool). As written, "Subsequent calls are no-ops" is technically true after the first call on any agent, but "once per agent instance" is misleading — a second, brand-new Agent(...) instance will not see the banner either, because the flag is process-wide. The matching docstring in _display.py:117 ("once per process") is the accurate description; align this one with it.
3 participants
Add a banner on first run of a Pydantic AI Agent