Conversation
📝 WalkthroughWalkthroughThis PR refactors GPU pricing calculations to use async generators and batch processing for deployments, introduces new GPU pricing statistics and model types, and adds comprehensive unit tests for the pricing service with modernized query patterns. Changes
Sequence DiagramsequenceDiagram
participant Controller as GPU Controller
participant Service as GpuPriceService
participant DepRepo as DeploymentRepository
participant DayRepo as DayRepository
Controller->>Service: getGpuModels()
Service->>DayRepo: getDaysAfter(date)
DayRepo-->>Service: days[]
Service->>Service: Create daysById Map
Service->>DepRepo: findAllWithGpuResources(options)
DepRepo-->>Service: async generator
loop For each batch of deployments
Service->>Service: Decode bids (v1beta4/v1beta5)
Service->>Service: Extract GPU features
Service->>Service: Match bids to GPU models
Service->>Service: Apply pricing logic<br/>(pricing-bot preference)
Service->>Service: Compute statistics<br/>(min, max, avg, med, weighted)
Service->>Service: Accumulate in gpusByModelAndVendor
end
Service->>Service: Build final result<br/>with aggregated availability
Service-->>Controller: GpuPriceModel[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/gpu/repositories/day.repository.ts (1)
1-8:⚠️ Potential issue | 🟠 MajorType annotation does not match return value—
raw: truereturns plain objects, notDayinstances.Line 7 declares
Promise<Day[]>, but line 8 usesraw: true, which returns plain attribute objects instead ofDaymodel instances. While the current caller only accesses simple properties (day.id,day.aktPrice), the type annotation is incorrect and will mislead future code that might attempt to use model-specific methods or relationships. Either removeraw: trueor update the return type to accurately reflect the plain object structure.🔧 Suggested fix
-import { Op } from "sequelize"; +import { Op, type Attributes } from "sequelize"; @@ - async getDaysAfter(date: Date): Promise<Day[]> { - return await Day.findAll({ where: { date: { [Op.gte]: date } }, raw: true }); + async getDaysAfter(date: Date): Promise<Array<Attributes<Day>>> { + return Day.findAll({ where: { date: { [Op.gte]: date } }, raw: true });Also update the caller in
gpu-price.service.ts:61to reflect the correct type:new Map<string, Attributes<Day>>()instead ofnew Map<string, Day>().apps/api/src/deployment/repositories/deployment/deployment.repository.ts (1)
109-135:⚠️ Potential issue | 🟠 MajorUse
separate: truefor nestedhasManyincludes to avoid duplicate rows, or manually deduplicate IDs.The nested
includefor DeploymentGroup → DeploymentGroupResource withraw: truecauses Sequelize to return one row per child combination. A deployment with multiple groups/resources appears duplicated, inflating the IDs array and causing redundant batch queries.Recommended approaches:
- Use
separate: true(preferred) — Sequelize fetches children in separate queries instead of joining, avoiding row multiplication:Suggested change
const recordsWithIds = await Deployment.findAll({ attributes: ["id"], where: { createdHeight: { [Op.gte]: options.minHeight } }, include: [ { attributes: [], model: DeploymentGroup, required: true, + separate: true, include: [ { attributes: [], model: DeploymentGroupResource, required: true, where: { gpuUnits: 1 } } ] } ], raw: true });
- Alternative: Deduplicate manually — If changing the query structure is not feasible, use
new Set():const ids = [...new Set(recordsWithIds.map(r => r.id))];
🤖 Fix all issues with AI agents
In `@apps/api/src/gpu/services/gpu-price.service.ts`:
- Around line 89-117: The code is using `any` for bid resource parsing (e.g., in
the `decodedBid.resourcesOffer` filter/flatMap/map chains and usages of
`resources.gpu.units.val`, `resources.cpu.units.val`,
`resources.memory.quantity.val`, `resources.storage`), which breaks type safety;
define a typed alias (e.g., `type ResourceOffer =
MsgCreateBidV4["resourcesOffer"][number] |
MsgCreateBidV5["resourcesOffer"][number]`) and cast `decodedBid.resourcesOffer`
to `ResourceOffer[]` once (e.g., `const resourcesOffer: ResourceOffer[] =
decodedBid.resourcesOffer`) then update the filter/flatMap/map callbacks to use
`ResourceOffer` instead of `any`, keeping references to `getGpusFromAttributes`,
and ensure the parsing helpers (`uint8arrayToString`, `parseInt`) operate on the
typed fields `resources.gpu.units.val`, `resources.cpu.units.val`,
`resources.memory.quantity.val`, and `resources.storage` so all four usages are
strongly typed and imported types (`MsgCreateBidV4`, `MsgCreateBidV5`) are
added.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2613 +/- ##
==========================================
- Coverage 50.39% 50.13% -0.26%
==========================================
Files 1029 1019 -10
Lines 29230 28896 -334
Branches 6657 6620 -37
==========================================
- Hits 14729 14488 -241
+ Misses 14210 14011 -199
- Partials 291 397 +106
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
was |
stalniy
force-pushed
the
refactor/gpu-prices
branch
from
af95bee to
6e74e21
Compare
stalniy
changed the title
2 participants
Why
perf
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.