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

Add New Integration for DeepInfra Embedding Model #13323

Merged
merged 20 commits into from May 13, 2024

Conversation

ovuruska
Copy link
Contributor

@ovuruska ovuruska commented May 7, 2024

Description
This pull request introduces a new integration for the DeepInfra Inference API.

Motivation and Context
Users are able to use DeepInfra's models using llama_index.

Fixes

  • No specific issue fixed; this is a new feature addition.

New Package?

  • Yes
  • No

If yes, I have filled in the tool.llamahub section in the pyproject.toml and provided a detailed README.md for my new integration.

Version Bump?

  • Yes
  • No

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Tests have been added to verify the functionality of the new embedding model integration. These tests ensure that both synchronous and asynchronous methods perform as expected.
  • Added new unit/integration tests
  • [x ] Added new notebook (that tests end-to-end)
  • I stared at the code and made sure it makes sense

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran make format; make lint to appease the lint gods

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label May 7, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ovuruska ovuruska marked this pull request as draft May 7, 2024 14:46
@ovuruska ovuruska marked this pull request as ready for review May 7, 2024 14:48
"""
self._model_id = model_id
self._normalize = normalize
self._api_token = os.getenv(ENV_VARIABLE, api_token)
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be api_token or os.getenv(..) ? Otherwise it ignores the api key in the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that makes sense. Thanks for the feedback.

self._text_prefix = text_prefix
self._batch_size = batch_size

super().__init__(callback_manager=callback_manager)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally I would call super first

self._api_token = os.getenv(ENV_VARIABLE, api_token)
self._query_prefix = query_prefix
self._text_prefix = text_prefix
self._batch_size = batch_size
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is already a parameter on the parent class for this embed_batch_size -- we should use that instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@logan-markewich Thanks for your feedback.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label May 13, 2024
@logan-markewich logan-markewich enabled auto-merge (squash) May 13, 2024 15:22
@logan-markewich logan-markewich merged commit 6e2ac5a into run-llama:main May 13, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants