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

Fix for embeddings #33

Merged
merged 3 commits into from
Dec 18, 2023
Merged

Fix for embeddings #33

merged 3 commits into from
Dec 18, 2023

Conversation

italianconcerto
Copy link
Contributor

Fixes on OpenAIEncoder

Copy link

PR Analysis

  • 🎯 Main theme: Fixing the embeddings in OpenAIEncoder and updating the version in pyproject.toml
  • 📝 PR summary: This PR introduces a fix for the embeddings in the OpenAIEncoder. It also updates the version in the pyproject.toml file. Additionally, it modifies the function_calling.ipynb notebook and the test_openai.py test file.
  • 📌 Type of PR: Bug fix
  • 🧪 Relevant tests added: No
  • ⏱️ Estimated effort to review [1-5]: 3, because the PR includes changes in different files including a Jupyter notebook, which might take more time to review.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR seems to be well-structured and the changes are logically grouped. However, it would be beneficial to add some tests to verify the changes made in the OpenAIEncoder. This would ensure that the changes work as expected and do not introduce new bugs.

  • 🤖 Code feedback:
    relevant filesemantic_router/encoders/openai.py
    suggestion      Consider handling the case where 'embeds' does not contain the 'data' key. This could be done by raising an appropriate exception. [important]
    relevant lineif "data" in embeds:

    relevant filedocs/examples/function_calling.ipynb
    suggestion      It is generally a good practice to avoid hardcoding values like 'gpt-4' in the code. Consider making it a parameter or a constant that can be easily changed. [medium]
    relevant line"def llm_openai(prompt: str, model: str = \"gpt-4\") -> str:\n",

    relevant filepyproject.toml
    suggestion      It's a good practice to keep a changelog that documents all the changes made in each version. This helps users and contributors understand what changes have been made in each version. [medium]
    relevant lineversion = "0.0.10"

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (45ce599) 100.00% compared to head (8011844) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #33   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines          344       344           
=========================================
  Hits           344       344           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@simjak simjak merged commit bfbf10b into main Dec 18, 2023
4 of 5 checks passed
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