Skip to content
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

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Address some comments for PR #5 #11

merged 1 commit into from
Jun 26, 2024

Conversation

yhwang
Copy link
Collaborator

@yhwang yhwang commented Jun 13, 2024

Derived from #5

  • 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
  • Remove some TODOs from the kubebuilder

@yhwang yhwang requested a review from gabe-l-hart June 13, 2024 08:43
@yhwang
Copy link
Collaborator Author

yhwang commented Jun 13, 2024

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

Copy link

@gabe-l-hart gabe-l-hart left a 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

api/v1beta1/groupversion_info.go Outdated Show resolved Hide resolved
backend/controller/lmevaljob_controller.go Outdated Show resolved Hide resolved
backend/controller/lmevaljob_controller.go Outdated Show resolved Hide resolved
backend/controller/lmevaljob_controller.go Outdated Show resolved Hide resolved
@yhwang yhwang force-pushed the address-comments branch 2 times, most recently from 201d983 to 75ec4f7 Compare June 25, 2024 15:44
@yhwang yhwang requested a review from gabe-l-hart June 25, 2024 17:19
Copy link

@gabe-l-hart gabe-l-hart left a 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 <yh.wang@ibm.com>
Copy link

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@yhwang
Copy link
Collaborator Author

yhwang commented Jun 26, 2024

thanks @gabe-l-hart

@yhwang yhwang merged commit 757bb44 into main Jun 26, 2024
3 checks passed
@yhwang yhwang deleted the address-comments branch June 26, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants