-
Notifications
You must be signed in to change notification settings - Fork 2
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
Address some comments for PR #5 #11
Conversation
some of the comments will be addressed by the corresponding issues. I will also create an issue to add test cases and enable GH action to run the test cases |
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.
A couple of NITs, but I think this is a nice set of fixes from the earlier PR
201d983
to
75ec4f7
Compare
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.
A couple more thoughts on the mapping and error semantics
- Add `image-pull-policy` in the ConfigMap to specify the imagePullPolicy of the job's pod - Add `pod-checking-interval` in the ConfigMap to set the pod checking interval - Update the Group and Kind of the CRD to `foundation-model-stack.github.com.github.com` and `LMEvalJob` - Refine the `checkScheduledPod` func of the controller based on the comment Signed-off-by: Yihong Wang <[email protected]>
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.
Looks good!
thanks @gabe-l-hart |
Derived from #5
image-pull-policy
in the ConfigMap to specify the imagePullPolicy of the job's podpod-checking-interval
in the ConfigMap to set the pod checking intervalfoundation-model-stack.github.com.github.com
andLMEvalJob
checkScheduledPod
func of the controller based on the comment