feat: Add new apiVersion with redesigned DGDR - #6095
Conversation
Signed-off-by: Jont828 <jt572@cornell.edu>
Signed-off-by: Jont828 <jt572@cornell.edu>
|
👋 Hi Jont828! Thank you for contributing to ai-dynamo/dynamo. Just a reminder: The 🚀 |
…conditions, and print cols to DGDR status Signed-off-by: Jont828 <jt572@cornell.edu>
| DGDRPhaseReady DGDRPhase = "Ready" | ||
| DGDRPhaseDeploying DGDRPhase = "Deploying" | ||
| DGDRPhaseDeployed DGDRPhase = "Deployed" |
There was a problem hiding this comment.
After looking at this, I think Ready isn't the best name for "successful profiling". Maybe we should change the phases:
- Pending
- Profiling
- ProfilingComplete (this is where it stops with
autoApply: false) - Deploying
- Deployed (this is where it stops with
autoApply: true) - Failed
There was a problem hiding this comment.
I think "ProfilingSucceeded" or "ProfilingComplete" is a good name. Should there be a planner phase, or is Deploying/Deployed enough?
There was a problem hiding this comment.
It's a DynamoDeployment, may or may not have Planner. So I don't think any phase should explicitly mention planner. Also DynamoDeploymentDeploying sounds kinda weird to me lol
There was a problem hiding this comment.
Oh I see, I didn't realize the planner was a component instead of a step in DGDR controller. Should we have some kind of aggregate/overall terminal phase similar to the "succeeded" condition? I think that would make it more clear that it's done and we can check against that value if we're deploying or not.
| // ConditionTypeSucceeded is the aggregate condition for the DGDR lifecycle. | ||
| // True = pipeline completed successfully; False = in progress or failed. | ||
| // Reason and Message reflect the current stage or error. | ||
| ConditionTypeSucceeded = "Succeeded" |
There was a problem hiding this comment.
For the records: this will happen if ProfilingComplete and autoApply: false or Deployed and autoApply: true
| EventReasonValidationFailed = "ValidationFailed" | ||
| EventReasonProfilingJobCreated = "ProfilingJobCreated" | ||
| EventReasonProfilingJobFailed = "ProfilingJobFailed" | ||
| EventReasonAIConfiguratorFailed = "AIConfiguratorFailed" |
There was a problem hiding this comment.
Not sure if this should be tied into/part of ProfilingJobFailed? Does the user need to know abt AIConfigurator explicitly
| ProfilingPhaseDone ProfilingPhase = "Done" | ||
| ) | ||
|
|
||
| // Profiling condition Reasons. |
There was a problem hiding this comment.
Should these be a bit more explanatory? @tedzhouhk
There was a problem hiding this comment.
agreed, is it possible to show more details, like sweeping prefill 2/4
There was a problem hiding this comment.
The 'Reason' needs to be a hard coded string. But each condition will have a message too, so I was thinking we could expose it in the message part, that way we don't need to say how many GPUs we're targeting for ex. Does that work?
Signed-off-by: Jont828 <jt572@cornell.edu>
Move the +kubebuilder:storageversion marker from v1alpha1 to v1beta1 for all CRDs and regenerate manifests so v1beta1 is the version persisted in etcd going forward. Signed-off-by: Jont828 <jt572@cornell.edu>
julienmancuso
left a comment
There was a problem hiding this comment.
hi @Jont828 what justifies the switch to using a new CRD version ?
internally we are having discussions but we are leaning towards keeping v1alpha1 for now.
let's hold on this change
Implement the controller-runtime conversion interface for 5 CRDs: DynamoCheckpoint, DynamoComponentDeployment, DynamoGraphDeployment, DynamoGraphDeploymentScalingAdapter, and DynamoModel. v1beta1 is marked as the Hub and v1alpha1 implements ConvertTo/ConvertFrom with field-by-field mapping. DGDR is excluded because the two versions have fundamentally different schemas that cannot be losslessly converted. Signed-off-by: Jont828 <jt572@cornell.edu>
|
currently we have two different definitions on the DGDR fields. One version is defined in this PR, another version is defined for the shim python layer #6285. We need to manually sync between them now. However, we should automate this in the future. |
| type OptimizationType string | ||
|
|
||
| const ( | ||
| OptimizationTypeHybrid OptimizationType = "hybrid" |
There was a problem hiding this comment.
Let's get rid of this option and make "throughput" the default
Align the Go CRD types with the Python profiler's pydantic models so that JSON serialized between operator and profiler is structurally consistent: - Introduce ModelSpec (modelName + modelCache) replacing flat model string and top-level modelCache field - Introduce BackendSpec (backend + dynamoImage) replacing flat backend enum and top-level image field - Rename ModelCacheSpec.PVCPath to ModelPathInPVC (json: modelPathInPvc) - Add WorkloadSpec.Concurrency and RequestRate fields - Add SLASpec.E2ELatency field - Replace FeaturesSpec.Planner *bool with *PlannerSpec (enabled, plannerPreDeploymentSweeping, plannerArgsList) - Replace FeaturesSpec.Mocker *bool with *MockerSpec (enabled) - Add HardwareSpec with gpuSku, vramMb, totalGpus, numGpusPerNode - Add PlannerPreDeploymentSweepMode enum (none, rapid, thorough) - Remove OptimizationType "hybrid" value - Update printcolumn JSONPaths for new nesting - Update deepcopy methods for new and changed types Signed-off-by: Jonathan Zhou <hongkuanz@nvidia.com> Signed-off-by: Jont828 <jt572@cornell.edu>
|
@tedzhouhk I pushed some changes to sync it based on your Python code. Here's a summary of what I changed. Does this look right to you? There's some Go types that aren't in the Python for your PR so not sure if you want to add them or not. |
846fefd to
e824985
Compare
Add v1alpha1 ↔ v1beta1 conversion for DynamoGraphDeploymentRequest (DGDR) and split the monolithic conversion.go into per-type files matching the existing *_types.go naming convention. Hub marker: add DynamoGraphDeploymentRequest to v1beta1/hub.go. DGDR conversion handles three categories of field mappings: - Simple renames (Model, Backend, AutoApply, UseMocker, WorkersImage) - JSON blob ↔ structured fields (SLA ttft/itl, Workload isl/osl, ModelCache) with full-blob annotation for round-trip preservation - Annotation-based storage for v1alpha1 fields with no v1beta1 equivalent (ProfilerImage, ConfigMapRef, OutputPVC, etc.) State ↔ Phase mapping accounts for v1alpha1 "Ready" mapping to either Ready or Deployed depending on Deployment.Created context. Signed-off-by: Jont828 <jt572@cornell.edu>
e824985 to
ae509a8
Compare
|
Closing as covered in #6352 |
Overview:
Work in progress following this proposal by @hhzhang16 .
Details:
Where should the reviewer start?
Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)