Reuse OpenAI clients to improve throughput

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏

[luojiyin1987]

Summary

• Add module-level singleton clients cached per API key
• Replace client creation with _get_sync_client/_get_async_client helper functions
• Fix chat_history mutation side effect (use list concatenation instead of append)
• Fix error return type inconsistency in ChatGPT_API_with_finish_reason
• Add missing re import

Closes#71

[murthi1832-cmd]

https://github.com/VectifyAI/PageIndex.git

[ncurado]

Nice improvement on client reuse + chat_history immutability. One small concern: _normalize_api_key returns "" when no key is set, so we end up caching a client under the empty-string key. Suggest guarding in _get_sync_client/_get_async_client (raise ValueError if normalized key is empty) or skip caching in that case, to avoid creating a client with an empty API key.

[KylinMountain]

Thanks @luojiyin1987! Since this was opened, the LLM layer was rewritten to route through LiteLLM (#168), so the functions this patches (ChatGPT_API* with direct openai.OpenAI clients) no longer exist on main and the PR now conflicts.

Each concern is already covered in the current code:

• Client reuse / throughput (Reuse OpenAI clients to improve throughput #71): LiteLLM reuses its underlying HTTP connections internally, so there's no per-call client creation anymore.
• chat_history mutation: llm_completion builds messages with list(chat_history) + [...], so the caller's list isn't mutated.
• Error return consistency: it returns "", "error" on max retries.
• Missing re import: landed via Adds missing re import #281.

Closing as superseded by the LiteLLM migration — thanks for the solid fixes, they're all reflected in main now. 🙏