-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Flex working1 #7045
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
Flex working1 #7045
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.
Thank you for your contribution! I've reviewed the changes and found several issues that need attention before this can be merged.
The main concerns are:
- Missing linked GitHub issue (required by contribution guidelines)
- Unrelated changes mixed with the flex pricing feature
- Debug code left in production
- Type safety issues that should be addressed
Please see my inline comments for specific suggestions.
|
||
const isSupported = useMemo(() => { | ||
const supported = !!effectiveModelId && FLEX_COMPATIBLE_MODELS.some((m) => effectiveModelId.includes(m)) | ||
console.log("[DEBUG] Service tier supported check:", { effectiveModelId, supported, FLEX_COMPATIBLE_MODELS }) |
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.
Debug console.log should be removed from production code. Consider using a proper logging utility if debugging is needed during development.
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.
Yes, agreed.
): number { | ||
const cacheCreationInputTokensNum = cacheCreationInputTokens || 0 | ||
const cacheReadInputTokensNum = cacheReadInputTokens || 0 | ||
const nonCachedInputTokens = Math.max(0, inputTokens - cacheCreationInputTokensNum - cacheReadInputTokensNum) | ||
|
||
// If flex tier selected and model exposes flexPrice, override pricing fields. | ||
const pricingInfo = | ||
serviceTier === "flex" && (modelInfo as any).flexPrice |
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 use of 'as any' bypasses TypeScript's type safety. Could we properly type the flexPrice property in the ModelInfo interface instead?
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.
agreed.
@@ -91,7 +91,6 @@ export function Features() { | |||
y: 0, | |||
transition: { | |||
duration: 0.6, | |||
ease: [0.21, 0.45, 0.27, 0.9], | |||
}, |
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.
These animation easing changes seem unrelated to the flex pricing feature. Could these be moved to a separate PR to keep this one focused on a single feature?
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.
These are in fact unrelated. These were causing compile errors for me. These cane be removed or ignored.
@@ -16,6 +16,11 @@ export const openAiNativeModels = { | |||
inputPrice: 1.25, | |||
outputPrice: 10.0, | |||
cacheReadsPrice: 0.13, | |||
flexPrice: { |
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 flexPrice object is missing cacheWritesPrice. Is this intentional, or should it include all pricing fields for consistency?
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.
OpenAI does not charged extra for CacheWrite. Check with the pricing of other OpenAI api model pricing, if cache write price is included, then inputPrice can be considered as cachWritesPrice, otherwise if these are not maintained for other openai models, then it can be safely ignored here.
@@ -1590,7 +1590,7 @@ export class McpHub { | |||
) | |||
} | |||
if (connection.server.disabled) { | |||
throw new Error(`Server "${serverName}" is disabled and cannot be used`) | |||
throw new Error(`Server \"${serverName}\" is disabled and cannot be used`) |
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 escaped quotes here are unnecessary since you're using double quotes for the string. This would be cleaner:
Server "" is disabled and cannot be used
Actually, looking at this again, the escaping is correct for the error message. Please disregard this 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.
Agreed. These should be removed.
738cd69
to
7c16a04
Compare
Create a new branch and a fresh clean pull request. CLosing this. Refer to PR 7079. CLosing this. |
Related GitHub Issue
Closes: #
Roo Code Task Context (Optional)
Description
Test Procedure
Pre-Submission Checklist
Screenshots / Videos
Documentation Updates
Additional Notes
Get in Touch
Important
Adds support for flexible pricing tiers in OpenAI cost calculations and updates UI components to handle service tier selection.
calculateApiCostOpenAI()
incost.ts
.calculateApiCostOpenAI()
to useflexPrice
ifserviceTier
is "flex".flexPrice
toModelInfo
inmodel.ts
andproviders/openai.ts
.ServiceTier
component inServiceTier.tsx
to select pricing tier.ApiOptions.tsx
andModelInfoView.tsx
to display and handle service tier selection.ModelPicker.tsx
to integrate service tier selection.calculateApiCostOpenAI()
with flex pricing incost.spec.ts
.settings.json
for service tier descriptions.This description was created by
for 7c16a04. You can customize this summary. It will automatically update as commits are pushed.