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

Complete conditional infinity import TODO #184

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

SecretiveShell
Copy link
Contributor

Why should this feature be added?

  • additional logging
  • changed declaration order prevents has_infinity_emb becoming undefined when there are errors other then import error
  • added additional type hinting

Examples
The software is more likely to crash without these changes if there is an issue with the infinity embed backend

Additional context
completes a TODO in the code

- add logging
- change declaration order
@bdashore3
Copy link
Member

bdashore3 commented Sep 4, 2024

I agree with a refactor of the error handling here, but I'd also like to remove the log statements for two reasons:

  1. The logger isn't set up at the time of import
  2. An import check isn't required unless a user wants to use embeddings, which the codebase already checks for. The import is at the top-level because of infinity's own logger which requires an issue on their end to make it configurable.

I can merge this after removing those logging statements and using a pass with the exception handler.

- remove logging statements
- format code with ruff
@SecretiveShell
Copy link
Contributor Author

should be ready for merge now, I made all the changes asked.

@bdashore3 bdashore3 merged commit 8524999 into theroyallab:main Sep 5, 2024
1 check 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