Skip to content
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

openai:compatible with other llm usage meta data #24500

Merged
merged 10 commits into from
Aug 23, 2024

Conversation

omgbbq
Copy link
Contributor

@omgbbq omgbbq commented Jul 22, 2024

  • PR message:

    • Description: Compatible with other llm (eg: deepseek-chat, glm-4) usage meta data
    • Issue: N/A
    • Dependencies: no new dependencies added
  • Add tests and docs:
    libs/partners/openai/tests/unit_tests/chat_models/test_base.py

cd libs/partners/openai
poetry run pytest tests/unit_tests/chat_models/test_base.py::test_openai_astream
poetry run pytest tests/unit_tests/chat_models/test_base.py::test_openai_stream
poetry run pytest tests/unit_tests/chat_models/test_base.py::test_deepseek_astream
poetry run pytest tests/unit_tests/chat_models/test_base.py::test_deepseek_stream
poetry run pytest tests/unit_tests/chat_models/test_base.py::test_glm4_astream
poetry run pytest tests/unit_tests/chat_models/test_base.py::test_glm4_stream

@efriis efriis added the partner label Jul 22, 2024
@efriis efriis self-assigned this Jul 22, 2024
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 22, 2024
Copy link

vercel bot commented Jul 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Aug 23, 2024 11:55pm

@dosubot dosubot bot added Ɑ: models Related to LLMs or chat model modules 🔌: openai Primarily related to OpenAI integrations 🤖:improvement Medium size change to existing code to handle new use-cases labels Jul 22, 2024
Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a test case where both content and usage_metadata are returned? I'm pretty sure this has the exact same bug as #23704

I think this if/else block would need more substantial refactoring to work with this.

Will mark as draft for now - feel free to mark ready again when that's fixed!

@efriis efriis marked this pull request as draft July 22, 2024 23:45
@omgbbq
Copy link
Contributor Author

omgbbq commented Jul 23, 2024

could you add a test case where both content and usage_metadata are returned? I'm pretty sure this has the exact same bug as #23704

I think this if/else block would need more substantial refactoring to work with this.

Will mark as draft for now - feel free to mark ready again when that's fixed!

@efriis Yes, the same one. Based on the original, the return stream chunk message and unit tests of chatglm4, deepseek and gpt-3.5-turbo are added in libs/partners/openai/tests/unit_tests/chat_models/test_base.py.

@omgbbq omgbbq requested a review from efriis July 30, 2024 03:05
@efriis efriis changed the title partner:compatible with other llm usage meta data openai:compatible with other llm usage meta data Jul 31, 2024
Copy link
Member

@efriis efriis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you fix the lint errors in the new tests?

hyman added 2 commits August 9, 2024 16:18
…_llm_usage

# Conflicts:
#	libs/partners/openai/tests/unit_tests/chat_models/test_base.py
@omgbbq omgbbq requested a review from efriis August 12, 2024 02:51
@efriis efriis marked this pull request as ready for review August 23, 2024 23:55
@efriis efriis enabled auto-merge (squash) August 23, 2024 23:55
@dosubot dosubot bot added the 🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs label Aug 23, 2024
@efriis efriis disabled auto-merge August 23, 2024 23:59
@efriis efriis merged commit 58e72fe into langchain-ai:master Aug 23, 2024
29 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖:improvement Medium size change to existing code to handle new use-cases Ɑ: models Related to LLMs or chat model modules 🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs 🔌: openai Primarily related to OpenAI integrations partner size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants