Repository navigation
feat(api): update API spec from langfuse/langfuse 14986f6 - #2049
langfuse-bot wants to merge 1 commit into
Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
No review was started: this request came from a bot account. Manual reviews can only be requested by someone with write access to this repository. Ask a maintainer to comment @claude review, or have your automation post the comment from a user account with write access.
Tip: disable this comment in your organization's Code Review settings.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| score_metadata: typing.Callable[[], T_Result], | ||
| score_provenance: typing.Callable[[], T_Result], |
There was a problem hiding this comment.
The two new required arguments break existing calls to BlobStorageExportFieldGroup.visit(), even for unchanged values such as CORE. A caller supplying all eleven callbacks required before this PR now gets TypeError before its callback runs. Make the new callbacks optional and check them only when visiting the new score groups.
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/api/blob_storage_integrations/types/blob_storage_export_field_group.py
Line: 44-45
Comment:
**Existing visitor calls fail**
The two new required arguments break existing calls to `BlobStorageExportFieldGroup.visit()`, even for unchanged values such as `CORE`. A caller supplying all eleven callbacks required before this PR now gets `TypeError` before its callback runs. Make the new callbacks optional and check them only when visiting the new score groups.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| @property | ||
| def skills(self): | ||
| if self._skills is None: | ||
| from .skills.client import SkillsClient # noqa: E402 |
There was a problem hiding this comment.
The new skills properties import their clients inside methods. The repository requires imports at the top of the module instead. Move these imports to module scope before merging. The async skills property and the topics properties in langfuse/api/unstable/client.py repeat this pattern.
Rule Used: Move imports to the top of the module instead of placing them within functions or methods. (source)
Learned From
langfuse/langfuse-python#1387
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/api/client.py
Line: 360
Comment:
**Imports break the repository rule**
The new `skills` properties import their clients inside methods. The repository requires imports at the top of the module instead. Move these imports to module scope before merging. The async `skills` property and the `topics` properties in `langfuse/api/unstable/client.py` repeat this pattern.
**Rule Used:** Move imports to the top of the module instead of placing them within functions or methods. ([source](https://app.greptile.com/personal-org-4986/-/custom-context?memory=c960fc07-9928-409f-a18b-a780cbdded12))
**Learned From**
[langfuse/langfuse-python#1387](https://github.com/langfuse/langfuse-python/pull/1387)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
| if _response.status_code == 400: | ||
| raise Error( |
There was a problem hiding this comment.
The second set of error branches cannot run: each matching status already raises an exception above, starting with 400 at line 112. This adds five misleading fallback branches to each of the eight request methods. Remove the duplicate branches and their unused imports, preferably in the generator, so maintainers do not have to read and update code that never runs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/api/unstable/topics/raw_client.py
Line: 189-190
Comment:
**Error fallbacks never run**
The second set of error branches cannot run: each matching status already raises an exception above, starting with `400` at line 112. This adds five misleading fallback branches to each of the eight request methods. Remove the duplicate branches and their unused imports, preferably in the generator, so maintainers do not have to read and update code that never runs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| def topics(self): | ||
| if self._topics is None: | ||
| from .topics.client import TopicsClient # noqa: E402 | ||
|
|
||
| self._skills = SkillsClient(client_wrapper=self._client_wrapper) | ||
| return self._skills | ||
| self._topics = TopicsClient(client_wrapper=self._client_wrapper) | ||
| return self._topics |
There was a problem hiding this comment.
馃煛 (optional) Code that calls api.unstable.skills gets an AttributeError after upgrading, and it gets no deprecation warning first. The skills property is removed from UnstableClient and AsyncUnstableClient, and the langfuse.api.unstable.skills module is gone. The moved client also only calls api/public/skills (langfuse/api/skills/raw_client.py:78), so self-hosted servers that only serve api/public/unstable/skills return 404. Fix: keep a deprecated unstable.skills alias (sync and async) in the hand-written layer or a Fern override, or document the break in the release notes and changelog. [also at: langfuse/api/skills/raw_client.py:78 - Self-hosted users on a Langfuse server older than upstream 14986f6 get 404 NotFoundError from every skills call after upgrading.]
Why this was flagged
On the base branch, LangfuseAPI(...).unstable.skills.list() resolves through UnstableClient.skills and sends GET api/public/unstable/skills. After this change, UnstableClient (langfuse/api/unstable/client.py:57) and AsyncUnstableClient only define topics. unstable.skills now raises AttributeError, and from langfuse.api.unstable.skills import SkillVersion / from langfuse.api.unstable import SkillMeta raise ImportError, because the entries were dropped from langfuse/api/unstable/init.py. A repo-wide grep finds no shim or alias. The relocated RawSkillsClient hard-codes api/public/skills and api/public/skills/{skill_name} (langfuse/api/skills/raw_client.py:78, 319, 431). So even callers who migrate get 404s against self-hosted Langfuse servers older than upstream 14986f6, and no fallback to the unstable path exists. This is a semver-breaking removal inside a feat bump, with no deprecation path.
Verification: In the head, UnstableClient only defines dashboard_widgets, dashboards and topics (langfuse/api/unstable/client.py:56-62), and the same goes for AsyncUnstableClient. So client.unstable.skills now raises AttributeError, and from langfuse.api.unstable import SkillMeta fails with ImportError. Nothing deprecated is left in their place.
Generated from langfuse/langfuse
mainat 14986f6.Fix the broken
visit()signature and satisfy the import-placement rule before merging.Summary
Updates the generated API clients from the upstream spec.
Reviews (1) 路 Last reviewed commit: "feat(api): update API spec from langfuse..." 路 Reviewed by Greptile