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

support for CPU/GPU choice and initialization before starting the app #2

Merged
merged 3 commits into from
Sep 27, 2023

Conversation

Vokturz
Copy link
Contributor

@Vokturz Vokturz commented Sep 22, 2023

  • The device can be defined by env variable DEVICE
  • created an initialize_embeddings function which initialize the embeddings before starting the app

model: Optional[str],
input: Union[str, List[str]]
):
def initialize_embeddings(model: Optional[str] = None):
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for send me the PR.

It make sense to load the text embeddings model before the first request. I leave the code of loading the embedding model on-demand back then due to I haven't make up my mind should the API serve multiple models or single model. I think it is clear now that the API serve single text embeddings model is the way to go, the code is simpler. If another text embeddings model is needed, the user can simply deploy another instance of the API.

Hence, the model parameter no longer needed here:

Suggested change
def initialize_embeddings(model: Optional[str] = None):
def initialize_embeddings():


if __name__ == "__main__":
initialize_embeddings()
Copy link
Owner

Choose a reason for hiding this comment

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

I will move the initialize_embeddings() to create_app() as the current approach will break the code of aws.py.

Copy link
Owner

@limcheekin limcheekin left a comment

Choose a reason for hiding this comment

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

I will merge the PR and make some minor changes as per my comments.

@limcheekin limcheekin merged commit c12550d into limcheekin:main Sep 27, 2023
@limcheekin
Copy link
Owner

Just release the package at https://pypi.org/project/open-text-embeddings/.

Kindly let's me know if you think there are ways to improve it. This is the first python package I publish to pypi.

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