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

Classification langchain example #117

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

paul-paliychuk
Copy link
Contributor

@paul-paliychuk paul-paliychuk commented Feb 20, 2024

🚀 This PR description was created by Ellipsis for commit a494d3d.

Summary:

This PR introduces new classification chains for the LangChain application, updates server routes, adds real-world scenario classification, and includes minor changes in memory.py, memory_async.py, and client.py.

Key points:

  • Added two new classification chains in classification_chain.py.
  • Updated server.py to include routes for the new classification chains.
  • Added a new file real_world_classification.py for real-world scenario classification.
  • Minor changes in memory.py, memory_async.py, and client.py mainly involving reordering of import statements.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Looks good to me! Reviewed entire PR up to commit dfa1aec

Reviewed 160 lines of code across 2 files in 1 minute(s) and 26 second(s).

See details
  • Skipped files: 2 (please contact us to request support for these files): examples/langchain-langserve/poetry.lock, examples/langchain-langserve/pyproject.toml
  • Confidence threshold: 50%
  • Drafted 3 additional comments.
  • Workflow ID: wflow_pPxC8UMNPZcYrhcq
View 3 draft comments

These comments were drafted by Ellipsis, but were filtered out of the final review. They're included here so you can see our internal thought process and help you configure your ellipsis.yaml.

Drafted 3 comments under confidence threshold

Filtered comment at /examples/langchain-langserve/app/classification_chain.py:91

Notes: The PR author is adding a new feature to classify user input into different categories and respond accordingly. The code seems to be logically correct and follows the best practices. However, there is no error handling or validation for the user input. This could lead to unexpected errors if the user input is not as expected.

Consider adding validation for the user input. This could help prevent unexpected errors if the user input is not as expected.

Confidence changes required: 50%

Filtered comment at /examples/langchain-langserve/app/classification_chain.py:19

Notes: The PR author is using environment variables for sensitive data which is a good practice. However, there is no error handling if the environment variables are not set. This could lead to unexpected errors at runtime.

Consider adding error handling if the environment variables are not set. This could help prevent unexpected errors at runtime.

Confidence changes required: 50%

Filtered comment at /examples/langchain-langserve/app/classification_chain.py:68

Notes: The PR author is using a lambda function to classify the user input. This is a good practice as it allows for flexibility in the classification logic. However, the lambda function is case sensitive which could lead to incorrect classifications if the user input is not in the expected case.

Consider making the lambda function case insensitive. This could help prevent incorrect classifications if the user input is not in the expected case.

Confidence changes required: 50%


Something look wrong? Tag @ellipsis-dev in a comment, or customize the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

No problems found on commit 8bbc301


Something look wrong? Tag @ellipsis-dev in a comment, or customize the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Looks good to me! Incremental review on commit a494d3d

Reviewed 269 lines of code across 6 files in 2 minute(s) and 22 second(s).

See details
  • Skipped files: 0
  • Confidence threshold: 50%
  • Drafted 1 additional comments.
  • Workflow ID: wflow_w4GRUV7tTgUn4I32
View 1 draft comments

These comments were drafted by Ellipsis, but were filtered out of the final review. They're included here so you can see our internal thought process and help you configure your ellipsis.yaml.

Drafted 1 comments under confidence threshold

Filtered comment at examples/langchain-langserve/app/classification_chain.py:1

Notes: The changes in the PR are mostly related to code formatting and reordering of import statements. There are no logical changes or modifications in the functionality of the code. The changes are in line with PEP 8 style guide which recommends importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. The PR also introduces two new classification chains for the LangChain application and updates the server routes to include these chains. There is also a new file for real-world scenario classification. All these changes seem to be correctly implemented without any apparent issues.

The changes in this PR are mostly related to code formatting and reordering of import statements. There are no logical changes or modifications in the functionality of the code. The changes are in line with PEP 8 style guide which recommends importing standard libraries first, followed by third-party libraries, and then local application/library specific imports. The PR also introduces two new classification chains for the LangChain application and updates the server routes to include these chains. There is also a new file for real-world scenario classification. All these changes seem to be correctly implemented without any apparent issues.

Confidence changes required: 0%


Something look wrong? Tag @ellipsis-dev in a comment, or customize the ellipsis.yaml for this repository.

Generated with ❤️ by ellipsis.dev

@danielchalef
Copy link
Member

@paul-paliychuk do we want to update this example to use the new ecommerce dataset and demo approach?

@paul-paliychuk paul-paliychuk marked this pull request as draft March 11, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants