feat:compatible with Pageindex SDK#238
Conversation
saccharin98
left a comment
saccharin98
left a comment
There was a problem hiding this comment.
Codex review
I found two cloud-compatibility issues that are worth fixing before merge. The local tests pass (78 passed, 2 skipped), but both cases can still fail against the real API because they involve response/status semantics and a server-side streaming flag.
-
chat_completions(..., stream_metadata=True) only changes the local parser; it is never included in the request payload. The existing CloudBackend.query_stream sends stream_metadata=True when it wants metadata chunks, so this compatibility method can return raw chunks without the API ever being asked to emit metadata/citation/tool fields. Please pass the flag through when requested and update the payload assertion test accordingly.
-
delete_document() returns response.json() and _request() only accepts status code 200. A successful REST delete commonly comes back as 204 or as 200 with an empty body; in either case this compatibility method raises instead of reporting success. The existing CloudBackend.delete_document intentionally ignores the body, so this should probably accept 2xx/empty responses and return an empty dict or otherwise special-case deletes.
|
Codex review follow-up correction I rechecked this PR against the old SDK implementation in pageindex_sdk-main. The two points I raised above should not be treated as blocking compatibility issues for this PR:
Those may still be future hardening opportunities if the cloud API contract changes, but they are not valid request-changes findings for this compatibility PR. Apologies for the earlier over-broad review. |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
- LegacyCloudAPI: close response in `finally` for both _stream_chat_response variants so abandoned iterators no longer leak the TCP connection. - PageIndexClient: emit a warning instead of silently falling back to local when api_key is the empty string, surfacing typical env-var-unset misconfig. - FakeResponse: add close()/closed to match the real requests.Response API. - Add unit coverage for stream close (both paths) and the empty-api_key warning. - Add scripts/e2e_legacy_sdk.py to smoke-test the legacy SDK contract end-to-end against api.pageindex.ai.
- Decorate the 12 PageIndexClient cloud-SDK compat methods with
@typing_extensions.deprecated(..., category=PendingDeprecationWarning):
- IDE/type-checkers render them with a strikethrough hint
- runtime warnings stay silent by default (no spam for existing callers),
surfaceable via `python -W default::PendingDeprecationWarning`
- Add a one-line docstring on each pointing to the Collection-based equivalent.
- Promote typing-extensions to a direct dependency (was transitive via litellm).
2 participants
No description provided.