-
Notifications
You must be signed in to change notification settings - Fork 310
docs: upgrade docstrings as a first-class documentation surface for AI agents #1760
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
7be4afe
0a6479a
04bdfa5
acc8c83
771119c
c0fa677
17dbd56
63503c6
5cce572
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 |
|---|---|---|
|
|
@@ -248,13 +248,13 @@ | |
| Example: | ||
| ```python | ||
| from langfuse.otel import Langfuse | ||
| from langfuse import Langfuse | ||
| # Initialize the client (reads from env vars if not provided) | ||
| langfuse = Langfuse( | ||
| public_key="your-public-key", | ||
| secret_key="your-secret-key", | ||
| host="https://cloud.langfuse.com", # Optional, default shown | ||
| base_url="https://cloud.langfuse.com", # Optional, default shown | ||
| ) | ||
| # Create a trace span | ||
|
|
@@ -420,7 +420,44 @@ | |
| ) | ||
|
|
||
| @property | ||
| def api(self) -> LangfuseAPI: | ||
| """Synchronous client for the full Langfuse REST API (traces, observations, scores, datasets, prompts, ...). | ||
| Use this to read or manage data on the Langfuse server; use the tracing methods | ||
| (`start_observation`, `@observe`) to create traces. Use `async_api` for the | ||
| asyncio variant. | ||
|
Check warning on line 428 in langfuse/_client/client.py
|
||
| Semantics that are easy to miss: | ||
| - **Ingestion is asynchronous.** `langfuse.flush()` only guarantees delivery to | ||
| the API, not read visibility: reads such as `api.trace.get(trace_id)` may | ||
| raise `langfuse.api.NotFoundError` until processing completes (typically | ||
| within 15-30 seconds; longer under load). The same applies to scores and | ||
| dataset run reads. Instead of a fixed sleep, retry with a deadline: | ||
|
Comment on lines
+432
to
+436
Contributor
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.
The bullet ends with "Instead of a fixed sleep, retry with a deadline:" (colon implies a code block follows), but the next line is the next bullet point. The retry snippet that was meant to appear here was described in the PR as a key addition for agents, but it was never included. Any agent or developer reading this will be left without the concrete pattern to copy. The Prompt To Fix With AIThis is a comment left during a code review.
Path: langfuse/_client/client.py
Line: 432-436
Comment:
**Dangling sentence — promised retry example is missing**
The bullet ends with "Instead of a fixed sleep, retry with a deadline:" (colon implies a code block follows), but the next line is the next bullet point. The retry snippet that was meant to appear here was described in the PR as a key addition for agents, but it was never included. Any agent or developer reading this will be left without the concrete pattern to copy. The `flush()` docstring also cross-references "the `api` property docs for a bounded retry pattern", pointing at this non-existent snippet.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
|
Check warning on line 437 in langfuse/_client/client.py
|
||
|
Comment on lines
+433
to
+437
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. 🟡 The Extended reasoning...The bug: The new A trailing colon here unambiguously signals that a code example follows — that's the standard convention this same PR uses elsewhere (e.g. the Corroborating evidence this isn't just a stray colon: Two independent signals confirm a snippet was intended and simply never written. First, the PR description explicitly states the Why nothing else catches this: This is purely docstring text with no executable code, so none of the PR's own verification steps surface it — Impact: This PR's stated goal is to make docstrings a reliable, first-class documentation surface that AI coding agents and humans read via Step-by-step proof:
Fix: Either add the promised bounded-retry example (e.g. a small |
||
| - **List endpoints return lightweight views.** `api.trace.list(...)` returns | ||
| `TraceWithDetails`, where `observations` and `scores` are lists of ID strings. | ||
| Fetch the full objects with `api.trace.get(trace_id)` (`TraceWithFullDetails`), | ||
| or prefer `api.observations.get_many(trace_id=...)` for row-level observation | ||
| queries. The same list-view vs. get-detail pattern applies to other resources. | ||
| - **Prefer the v2 data APIs — they are the defaults since SDK v4.** | ||
| `api.observations` and `api.metrics` map to the high-performance | ||
| `/api/public/v2/...` endpoints and are the recommended read path. Their v1 | ||
| equivalents remain available under `api.legacy.observations_v1` / | ||
| `api.legacy.metrics_v1` but are less performant at scale, not recommended | ||
| for new workflows, and will be deprecated. | ||
| - For large-scale aggregation (usage/cost by model, user, etc.), prefer the | ||
| v2 Metrics API (`api.metrics.metrics(...)`) over paginating row-level data. | ||
| See also: `async_api`, | ||
| https://langfuse.com/docs/api-and-data-platform/features/query-via-sdk | ||
| (ingestion lag: #ingestion-lag, list vs. get: #traces-list-vs-get), | ||
| https://langfuse.com/docs/api-and-data-platform/features/observations-api, | ||
| https://langfuse.com/docs/metrics/features/metrics-api | ||
| """ | ||
| if self._resources is None: | ||
| raise AttributeError("Langfuse client is not initialized") | ||
|
|
||
|
|
@@ -1519,7 +1556,7 @@ | |
| evaluators). It will be removed in a future major version. | ||
| For setting other trace attributes (user_id, session_id, metadata, tags, version), | ||
| use :meth:`propagate_attributes` instead. | ||
| use :func:`langfuse.propagate_attributes` (top-level import) instead. | ||
| Args: | ||
| input: Input data to associate with the trace. | ||
|
|
@@ -2239,6 +2276,16 @@ | |
| # Continue with other work | ||
| ``` | ||
| Note: | ||
| `flush()` guarantees data was *delivered* to the API, not that it is | ||
| *readable* yet: server-side ingestion is asynchronous, so flushed data | ||
| may not be queryable for 15-30 seconds — | ||
| `api.observations.get_many(trace_id=...)` may return empty results and | ||
| `api.trace.get()` may raise `langfuse.api.NotFoundError` right after a | ||
| successful flush. See the `api` property docs for a bounded retry | ||
| pattern, or | ||
| https://langfuse.com/docs/api-and-data-platform/features/query-via-sdk#ingestion-lag | ||
| """ | ||
| if self._resources is not None: | ||
| self._resources.flush() | ||
|
|
||
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 PR description says property docstrings were added for both
Langfuse.apiandLangfuse.async_api(including scoresget_many/get_by_idguidance), but onlyapi(client.py:423-460) actually received one —async_api(client.py:473-478) remains undocumented, and the scores guidance the description mentions doesn't appear anywhere in the file.Extended reasoning...
The PR's stated goal is to make the SDK's docstrings a first-class documentation surface, and its description explicitly frames the
api/async_apiwork as a pair: "Langfuse.api / async_api — new property docstrings (previously none)", going on to list several semantics it claims were documented, including "scores are read via get_many/get_by_id — there is no api.scores.get".In the actual diff, only the
apiproperty (client.py:423-460) gets the new docstring covering ingestion lag, list-vs-get semantics, and the v2 metrics/observations APIs. Theasync_apiproperty, defined a few lines below right after theapisetter (client.py:473-478), is completely unchanged by this PR and still has no docstring at all — its body goes straight from the@propertydecorator to theif self._resources is Noneguard.help(langfuse.async_api)/inspect.getdoc(langfuse.async_api)returnNone, while the same calls onlangfuse.apinow return the new text. Additionally, grepping client.py confirms the promised scoresget_many/get_by_id/no-scores.getguidance was never actually written into either docstring — the newapidocstring only mentionsapi.observations.get_many, not scores.Concretely: (1) open client.py, jump to line 423 — the new docstring is present on
api. (2) Scroll to line 473 —async_api's body has no docstring statement whatsoever. (3) Runimport inspect; from langfuse import Langfuse; inspect.getdoc(Langfuse.async_api)— it returnsNone, contradicting the PR description's claim that both properties got documented. (4) Search the whole file for 'get_by_id' or 'scores are read' — no match, confirming that specific claimed addition was never written.Since
async_apiis a first-class, identically-used surface for asyncio consumers (same underlying REST client, same ingestion-lag/list-vs-get caveats apply), leaving it silently undocumented while the sync twin gets detailed guidance is a real, in-scope completeness gap for a PR whose entire purpose is upgrading exactly this kind of documentation. It is not a functional defect — nothing crashes or behaves incorrectly at runtime, and theapidocstring does at least point readers toasync_apias 'the asyncio variant' — so it doesn't block merging. The fix is trivial: mirror theapidocstring (or a short pointer with a note about awaiting the async client methods) ontoasync_api, and add the scoresget_many/get_by_idguidance that was described but never written.