Repository navigation
feat(rod): extract requested resources from work units using domain resources adapter - #407
Merged
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new resourceAccumulator utility and corresponding tests to extract and aggregate requested resources (CPU, memory, GPU, and TPU) from Kubernetes jobs within a work unit. It also integrates this utility into jobs.go to populate the RequestedResources field. The review feedback highlights a critical issue where the accumulator currently ignores the replica count of each job. The reviewer suggests updating the accumulator helper methods to accept a replica multiplier, adjusting the job resource accumulation logic accordingly, and updating the unit tests to reflect these changes.
janzak-dev
marked this pull request as draft
August 10, 2026 08:14
janzak-dev
force-pushed
the
feat/rod-pr-8-1b-resource-accumulator
branch
3 times, most recently
from
August 10, 2026 08:39
5a3a24c to
6ada843
Compare
janzak-dev
force-pushed
the
feat/rod-pr-8-1b-resource-accumulator
branch
2 times, most recently
from
August 10, 2026 10:12
c744491 to
f7a2dcd
Compare
janzak-dev
force-pushed
the
feat/rod-pr-8-1-mocks-and-dto
branch
from
August 10, 2026 10:28
0403849 to
9e57844
Compare
janzak-dev
force-pushed
the
feat/rod-pr-8-1b-resource-accumulator
branch
from
August 10, 2026 10:29
d7e2cc0 to
185ac8a
Compare
janzak-dev
force-pushed
the
feat/rod-pr-8-1-mocks-and-dto
branch
from
August 10, 2026 11:04
405c7c7 to
fddfe0b
Compare
janzak-dev
force-pushed
the
feat/rod-pr-8-1b-resource-accumulator
branch
from
August 10, 2026 11:04
119b36b to
d37df4f
Compare
…tx checks, and expand unit test coverage
… ExecutableSpec.resources
…om ResourceSet without job loop or replica multiplier
…replica multiplier
…er methods and unit tests
…t.GetWorkUnitResources adapter
… expand table-driven test suite
- Implement QueueJobResourcesPipe to map DashboardJob.requestedResources to ResourceChipData array (CPU cores, memory bytes PB/GB, GPU/TPU accelerators). - Add Resources column to QueueJobsTable standalone component, template, and SCSS layout. - Update QueueJobsTableHarness, queue_jobs_pipes_test.ts, and queue_jobs_table_test.ts. TAG=agy
TAG=agy CONV=6109e3eb-6351-4dc2-a259-9440069fd8ea
…ce[] - Change QueueJobResourcesPipe.transform signature to accept strictly resources?: Resource[] | null. - Update template binding in queue_jobs_table.ng.html to pass job.requestedResources | queueJobResources. - Update unit tests in queue_jobs_pipes_test.ts. TAG=agy CONV=6109e3eb-6351-4dc2-a259-9440069fd8ea
…WorkUnitResources adapter
janzak-dev
force-pushed
the
feat/rod-pr-8-1b-resource-accumulator
branch
from
August 10, 2026 11:42
cab7c62 to
5c0f73d
Compare
…PU topology parsing, and pre-allocate map capacity
janzak-dev
marked this pull request as ready for review
August 10, 2026 12:26
janzak-dev
requested review from
VLukiantsev
and removed request for
pachopepe
August 10, 2026 12:27
VLukiantsev
reviewed
Aug 10, 2026
VLukiantsev
reviewed
Aug 10, 2026
VLukiantsev
reviewed
Aug 10, 2026
VLukiantsev
reviewed
Aug 10, 2026
VLukiantsev
reviewed
Aug 10, 2026
VLukiantsev
reviewed
Aug 10, 2026
Collaborator
There was a problem hiding this comment.
Thank you for working on this! Overall the feature implementation is clean and well-structured. I have posted inline feedback items regarding named return values, simplifying closure indirection, handling TPU resource types in the UI, ensuring TotalQueueSize accuracy, and expanding unit test coverage.
janzak-dev
enabled auto-merge
August 10, 2026 13:56
VLukiantsev
approved these changes
Aug 10, 2026
AntonTyb
pushed a commit
that referenced
this pull request
Aug 11, 2026
…esources adapter (#407) * refactor(rod): populate requested_resources, shorten functions, add ctx checks, and expand unit test coverage * refactor(rod): extractRequestedResourcesFromSpec directly operates on ExecutableSpec.resources * refactor(rod): simplify extractRequestedResources to read directly from ResourceSet without job loop or replica multiplier * refactor(rod): use type switch on ResourceSet.resources in extractRequestedResources * refactor(rod): use GPU and TPU as default resource type strings * refactor(rod): aggregate resources across all jobs in jobset without replica multiplier * refactor(rod): extract resourceAccumulator struct with dedicated helper methods and unit tests * refactor(rod): refactor jobs_test.go to use proto.Modify and baseline fixtures * feat(rod): extract resourceAccumulator to separate file and test suite * refactor(rod): replace custom resourceAccumulator with domain workunit.GetWorkUnitResources adapter * fix(rod): parse TPU topology dimensions to calculate total TPU chips count * test(rod): add unit tests for TPU resource extraction and topology chip calculation * refactor(rod): decompose extractRequestedResources under 15 lines and expand table-driven test suite * feat(rod): render Resources column in Angular UI for ROD jobs table - Implement QueueJobResourcesPipe to map DashboardJob.requestedResources to ResourceChipData array (CPU cores, memory bytes PB/GB, GPU/TPU accelerators). - Add Resources column to QueueJobsTable standalone component, template, and SCSS layout. - Update QueueJobsTableHarness, queue_jobs_pipes_test.ts, and queue_jobs_table_test.ts. TAG=agy * refactor(ui): simplify resource formatting logic in queue jobs table TAG=agy CONV=6109e3eb-6351-4dc2-a259-9440069fd8ea * refactor(ui): restrict QueueJobResourcesPipe input strictly to Resource[] - Change QueueJobResourcesPipe.transform signature to accept strictly resources?: Resource[] | null. - Update template binding in queue_jobs_table.ng.html to pass job.requestedResources | queueJobResources. - Update unit tests in queue_jobs_pipes_test.ts. TAG=agy CONV=6109e3eb-6351-4dc2-a259-9440069fd8ea * refactor(rod): sync jobs.go and jobs_test.go with domain workunit.GetWorkUnitResources adapter * refactor(rod): format accelerators as ${count} x ${type}, normalize TPU topology parsing, and pre-allocate map capacity * refactor(rod): extract resource type string literals into constants * refactor(rod): address PR 407 review comments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds requested resource extraction (
extractRequestedResources) tocmd/rod_data_provider, populating theRequestedResourcesDTO field onDashboardJobprotobufs.It leverages the domain layer's
workunit.GetWorkUnitResourcesto aggregate resources across job specs, replicas, sidecars, and init containers, convertingResourceSetmessages intospb.ResourceDTOs.Key Changes
cmd/rod_data_provider/resource_accumulator.go):extractRequestedResourcesto convert domainResourceSetobjects fromworkunit.GetWorkUnitResources(wu)into presentation DTOs ([]*spb.Resource).float64(millicores) / 1000.0), Memory bytes, GPU type counts, and TPU topology chip calculations (parseTotalChips).[]*spb.Resource).accumulateResourceTotals,buildResourceDTOs,accumulateCpuResource,accumulateGpuResource,accumulateTpuResource,parseTotalChips).cmd/rod_data_provider/jobs.go): PopulatedRequestedResources: extractRequestedResources(wu)inmapWorkUnitToDashboardJob.cmd/rod_data_provider/resource_accumulator_test.go): Added a comprehensive table-driven test suite (TestExtractRequestedResourcesandTestParseTotalChips) covering CPU, Memory, GPU type fallbacks, TPU topology chip math (2x2x4-> 32 chips), andnil/emptywork units.cmd/rod_data_provider/BUILD.bazel): Added"//internal/domain/workunit"to:rod_data_provider_lib.Verification
bazel test //internal/storage/spanner/workunit:workunit_unit_test //cmd/rod_data_provider/...— PASSED 100% (6 of 6 targets).golangci-lintandbuildifier— PASSED.Screenshot