-
Notifications
You must be signed in to change notification settings - Fork 622
Ai provider update #5254
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?
Ai provider update #5254
Conversation
boutell
left a comment
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.
This is very cool stuff.
I'm pushing hard to get it right because I can see us pivoting this into core as the way we interface to models in general.
packages/ai-helper/index.js
Outdated
| // APOS_AI_HELPER_LOG_USAGE env var | ||
| // When true, usage data is logged to the console | ||
| // for cost tracking and auditing | ||
| logUsage: process.env.APOS_AI_HELPER_LOG_USAGE === 'true' || false, |
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.
Just check it for truthiness IMHO, but if not we must support 1 in particular for consistency with common practice including our own.
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.
logUsage eliminated.
packages/ai-helper/index.js
Outdated
| self.aposAiHelperUsage = self.apos.db.collection('aposAiHelperUsage'); | ||
| await self.aposAiHelperUsage.createIndex( | ||
| { createdAt: -1 }, | ||
| { name: 'createdAt_-1' } |
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.
I'd remove this as it's not better than the default convention (I think it might be the default convention).
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.
Changed.
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.
Removed.
packages/ai-helper/index.js
Outdated
| userId: 1, | ||
| createdAt: -1 | ||
| }, | ||
| { name: 'userId_1_createdAt_-1' } |
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.
Same re: not adding name if it's just the same as the default convention, which is fine
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.
Since we are removing the storage, this goes away.
| modules: getBundleModuleNames() | ||
| }, | ||
| options: { | ||
| textModel: 'gpt-5.1', |
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.
We have bc provisions I assume?
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.
Provisions added.
packages/ai-helper/index.js
Outdated
| * @param {boolean} [config.image] - Supports image generation | ||
| * @param {boolean} [config.imageVariation] - Supports image variations | ||
| */ | ||
| registerProvider(provider, config) { |
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.
I actually think a full apostrophecms module is overkill for a provider. Not worth the startup overhead. I would go with passing an ordinary object. It's fine if it happens to be an apostrophe module.
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.
I recognize people will want to override options of the individual providers, but they only need to do that for the ones they are actually using. So if you add textProviderOptions and imageProviderOptions to the main module that should address that concern.
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.
Switched to factories.
| @@ -0,0 +1,120 @@ | |||
| module.exports = { | |||
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.
I think it should be an object and not a full apos module.
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.
Developers can call registerProvider in project level init of the ai-helper module and pass their own provider objects. The main init would call it once for each of the standard providers.
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.
Changed.
|
|
||
| // Retry logic for transient failures | ||
| let lastError; | ||
| for (let attempt = 1; attempt <= 3; attempt++) { |
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.
Make the number of attempts configurable.
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.
Added and documented as an option.
| lastError = e; | ||
|
|
||
| // Log for debugging | ||
| console.error(`Anthropic request failed (attempt ${attempt}/3):`, e.message); |
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.
Providers should be registered as functions that accept a services object with more appropriate logging functions (e.g. access to structured logging).
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.
Done.
| const url = `https://generativelanguage.googleapis.com/v1beta/models/${model}:generateContent`; | ||
|
|
||
| // Gemini returns one image per request, | ||
| // so make multiple requests for multiple images |
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.
Parallel requests?
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.
Refactored for parallel requests.
| // Default text generation settings | ||
| textMaxTokens: 1000, | ||
| imageModel: 'gpt-image-1-mini', | ||
| imageSize: '1024x1024', |
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.
I didn't check, but make sure these are standardized across providers whenever it makes sense.
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.
Option names were standardized where possible.
|
|
||
| const result = await provider.module.generateText(req, prompt, { | ||
| const result = await provider.generateText(req, prompt, { | ||
| maxTokens: aiHelper.options.textMaxTokens, |
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.
I thought you moved this into a textProviderOptions object.
packages/ai-helper/index.js
Outdated
| const imageProvider = self.options.imageProvider; | ||
|
|
||
| // Register bundled providers | ||
| self.registerProvider(openaiProvider( |
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.
I think we're trying too hard to provide for multiple text providers, multiple image providers. We don't have a proven need for that yet, so we shouldn't prematurely implement it.
Registering a provider shouldn't immediately instantiate it. It just puts it on the list of candidates that are valid for the textProvider and imageProvider options.
I think this flow makes sense:
- In
init, callself.registerProviders, which callsregisterProviderfor the built-in choices, and is intended to be extended viaextendMethodsto register more providers - Then call
activateProviders, which activates only the needed providers (one for text and one for images)
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.
Looking at the providers, it's pretty cheap to instantiate. We basically store a simple object representing the meta AND the interface, it's quite smart. Splitting it to register and activate brings more chore IMO.
I always prefer the idea of explicit registration call (aiHelper.registerProvider(xxx)) vs extending (things can go south much easily).
| return { | ||
| post: { | ||
| async aiHelper(req) { | ||
| console.log('req.body', req.body); |
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.
Cleanup
myovchev
left a comment
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.
I'm giving a high level feedback - I like what I see. The provider contract is clean. It's flexible. I can see how even "non-active" providers (not pointed by the respective options) are still available and can be used in different scenarios programmatically (e.g. I can provide a runtime list in my custom project UI).
Few lint issue has to be fixed.
| }); | ||
| }, | ||
| createVariant() { | ||
| if (!this.variantPrompt.trim()) return; |
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.
Lint issue
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.
Fixed. (Hate that rule though.)
packages/ai-helper/index.js
Outdated
| * @returns {Array} Array of provider info | ||
| */ | ||
| listProviders() { | ||
| return Array.from(self.providers.entries()).map(([name, info]) => ({ |
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.
lint issue
Summary
Summarize the changes briefly, including which issue/ticket this resolves. If it closes an existing Github issue, include "Closes #[issue number]"
This PR refactors the AI helper module to use a provider module structure, rather than being hardcoded for OpenAI. It also creates provider files for OpenAI, Anthropic, and Gemini. Finally, it adds a "skills" file that devs can use to speed their creation of custom provider files.
I will still need help before acceptance with the correct steps for the CHANGELOG in the new mono-repo
What are the specific steps to test this change?
For example:
What kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: