-
Notifications
You must be signed in to change notification settings - Fork 79
standalone: Allow specifying controller image variant via env #213
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
Conversation
Reviewer's GuideIntroduced a centralized controller image naming module that resolves version and variant via environment variables, replacing scattered inline logic across image pulling, pruning, and container creation. Class diagram for new controller image naming helpersclassDiagram
class controller_image.go {
+ControllerImage : string
+defaultControllerImageVersion : string
+controllerImageVersion() string
+controllerImageVariant(detectedGPU gpupkg.GPUSupport) string
+fmtControllerImageName(repo string, version string, variant string) string
+controllerImageName(detectedGPU gpupkg.GPUSupport) string
}
Class diagram for refactored usage in images.go and containers.goclassDiagram
class images.go {
+EnsureControllerImage(ctx, dockerClient, gpu, printer) error
+PruneControllerImages(ctx, dockerClient, printer) error
}
class containers.go {
+CreateControllerContainer(ctx, dockerClient, port, environment, doNotTrack, gpu, modelStorageVolume, printer, engineKind) error
}
class controller_image.go
images.go --> controller_image.go : uses
containers.go --> controller_image.go : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
3da79cc to
28d3bea
Compare
|
|
||
| func fmtControllerImageName(repo, version, variant string) string { | ||
| tag := repo + ":" + version | ||
| if len(variant) > 0 { |
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.
nit:
| if len(variant) > 0 { | |
| if variant != "" { |
I guess the compiler produces the same output nowadays.
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider normalizing the MODEL_RUNNER_CONTROLLER_VARIANT value (e.g. trimming whitespace and lowercasing) before use to avoid unexpected tag mismatches.
- The special-casing of “cpu” and “generic” both mapping to no suffix may be confusing—either document that behavior clearly or adjust the logic to handle a distinct “generic” tag if needed.
- Add unit tests for the new controller_image.go helpers to validate expected behavior across different environment variable and GPU detection combinations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing the MODEL_RUNNER_CONTROLLER_VARIANT value (e.g. trimming whitespace and lowercasing) before use to avoid unexpected tag mismatches.
- The special-casing of “cpu” and “generic” both mapping to no suffix may be confusing—either document that behavior clearly or adjust the logic to handle a distinct “generic” tag if needed.
- Add unit tests for the new controller_image.go helpers to validate expected behavior across different environment variable and GPU detection combinations.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Piotr Stankiewicz <[email protected]>
28d3bea to
bc1483b
Compare
Summary by Sourcery
Simplify and centralize controller image handling by extracting version and variant logic into utilities and enabling environment-driven image variant configuration
New Features:
Enhancements: