-
Notifications
You must be signed in to change notification settings - Fork 629
integrated CrofAI (in theory) #1164
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CrofAI integration looks good overall, but I've identified a few issues that should be addressed to ensure proper functionality and maintainability.
Skipped files
cookbook/integrations/crofai.ipynb
: File hunk diff too large
src/providers/crofai/chatComplete.ts
Outdated
object: response.object, | ||
created: response.created, | ||
model: response.model, | ||
provider: DEEPINFRA, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: The provider is incorrectly set to DEEPINFRA in the response transformation.
Fix: Replace DEEPINFRA with a CrofAI-specific constant.
Impact: Ensures responses correctly identify CrofAI as the provider instead of DeepInfra.
provider: DEEPINFRA, | |
provider: DEEPINFRA, |
src/providers/crofai/chatComplete.ts
Outdated
object: parsedChunk.object, | ||
created: parsedChunk.created, | ||
model: parsedChunk.model, | ||
provider: DEEPINFRA, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: The provider is incorrectly set to DEEPINFRA in the stream chunk transformation.
Fix: Replace DEEPINFRA with a CrofAI-specific constant.
Impact: Ensures stream responses correctly identify CrofAI as the provider instead of DeepInfra.
provider: DEEPINFRA, | |
provider: DEEPINFRA, |
Hey @scuzzles could you add some documentation links? |
documentation for the code I've commited or documentation for CrofAI? |
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
@scuzzles - Can you look at the PR comments from matter? |
for CrofAI |
|
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Could be wrong but I believe they should be fixed or committed now |
Description
Should add CrofAI as a provider in portkey gateway
Motivation
Is the cheapest inference provider for almost every large LLM
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues