8000 Add inftyai-scheduler support and config updates by carlory · Pull Request #447 · InftyAI/llmaz · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add inftyai-scheduler support and config updates #447

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 10, 2025
Merged

Add inftyai-scheduler support and config updates #447

merged 2 commits into from
Jun 10, 2025

Conversation

carlory
Copy link
Member
@carlory carlory commented Jun 9, 2025 •
edited
Loading

What this PR does / why we need it

Add inftyai-scheduler support and config updates

Test it with a custom scheduler name.

(base) ➜  llmaz git:(scheduler-plugins) kubectl get cm llmaz-global-config demo-inftyai-scheduler-scheduler-config -oyaml | grep sched
    config.data: 'scheduler-name: demo-inftyai-scheduler'
    scheduler-config.yaml: |
      apiVersion: kubescheduler.config.k8s.io/v1
        resourceName: demo-inftyai-scheduler
      - schedulerName: demo-inftyai-scheduler
    name: demo-inftyai-scheduler-scheduler-config
(base) ➜  llmaz git:(scheduler-plugins) kubectl get po qwen2-0--5b-0 -oyaml | grep sched
  schedulerName: demo-inftyai-scheduler

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?

 Add inftyai-scheduler support and config updates

@InftyAI-Agent InftyAI-Agent added needs-triage Indicates an issue or PR lacks a label and requires one. needs-priority Indicates a PR lacks a label and requires one. do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Jun 9, 2025
@InftyAI-Agent InftyAI-Agent requested review from kerthcet June 9, 2025 06:56
@carlory carlory force-pushed the scheduler-plugins branch 2 times, most recently from 5aa2b0f to 719ccd4 Compare June 9, 2025 06:57
@carlory
Add inftyai-scheduler support and config updates
c6f907a 
Signed-off-by: carlory <baofa.fan@daocloud.io>
@carlory carlory force-pushed the scheduler-plugins branch from 719ccd4 to c6f907a Compare June 9, 2025 06:58
kerthcet
kerthcet reviewed Jun 9, 2025
View reviewed changes
Makefile Outdated
@@ -317,7 +317,9 @@ $(HELMIFY): $(LOCALBIN)

.PHONY: helm
helm: manifests kustomize helmify
$(KUSTOMIZE) build config/default | $(HELMIFY) -crd-dir
$(KUSTOMIZE) build config/default \
| yq 'select(.kind and (.kind != "ConfigMap" or .metadata.name != "llmaz-global-config"))' \
Copy link
Member

Choose a reason for hiding this comment

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

Better not to add too much logic to the makefile command, what's the advantages over the current one?

Copy link
Member Author
@carlory carlory Jun 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Because the default value is default-scheduler in config/default/configmap.yaml and it can not be easily changed by user via helm install --set command.

I can not edit chart/template/global-config.yaml because it is generated by helmify. I want users to be able to easily use the chart without extra configuration.

Copy link
Member

Choose a reason for hiding this comment

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

But it's not a general solution, we have a lot of configurations.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok to let user configure the configmaps via global.values.yaml. It's also not difficult.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we switch the default scheduler to inftyai-scheduler in config/default/configmap.yaml? And add make install-inftyai-scheduler and make uninstall-inftyai-scheduler to the Makefile.

install-inftyai-scheduler:
	kubectl apply --server-side -k config/inftyai-scheduler

.PHONY: uninstall-prometheus
uninstall-inftyai-scheduler:
	kubectl delete -k config/inftyai-scheduler

Copy link
Member Author
@carlory carlory Jun 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If not, we can not enable the inftyai-scheduler in chart by default due to the immutability of the configmap which is generated by helmify.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ok to let user configure the configmaps via global.values.yaml. It's also not difficult.

This pr has supported it.

kerthcet
kerthcet reviewed Jun 10, 2025
View reviewed changes
chart/templates/global-config.yaml Outdated
@@ -5,4 +5,14 @@ metadata:
labels:
{{- include "chart.labels" . | nindent 4 }}
data:
config.data: {{ .Values.globalConfig.configData | toYaml | indent 1 }}
config.data: |-
{{- $base := deepCopy .Values.globalConfig.configData }}
Copy link
Member

Choose a reason for hiding this comment

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

What I mean is we may have several configurations in globalconfig, so this is not a general solution, I would suggest revert to the original configurations, and people can modify the chart/values.global.yaml before installation/upgrade, does that workaround?

Copy link
Member Author

Choose a reason for hiding this comment

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

It works. Howerver, if an user want to use the scheduler, he have to do the following:

  1. Get values from chart via helm show values oci://registry-1.docker.io/inftyai/llmaz > /t mp/values.yaml

  2. Modify the values.yaml to enable the scheduler like this:

kube-scheduler:
  enabled: true

globalConfig:
  configData: |-
    scheduler-name: inftyai-scheduler
    # init-container-image: inftyai/model-loader:v0.0.10
  1. Install the chart with the modified values.yaml.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can not add this config to values.global.yaml in codebase because make helm-package appends (not merge) the file to values.yaml.

Copy link
Member Author
@carlory carlory Jun 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another drawback is that users need to provide the whole content of the configData when they want to change only one field because the globalConfig.configData is a string, not a map.

@kerthcet
Copy link
Member

let's merge for now and optimize it later.
/lgtm
/approve
/kind cleanup

Thanks @carlory

@InftyAI-Agent InftyAI-Agent added lgtm Looks good to me, indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a label and requires one. labels Jun 10, 2025
@InftyAI-Agent InftyAI-Agent merged commit 4a29db2 into InftyAI:main Jun 10, 2025
29 checks passed
@carlory carlory deleted the scheduler-plugins branch June 10, 2025 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kerthcet kerthcet kerthcet left review comments

Assignees

@kerthcet kerthcet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Looks good to me, indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a label and requires one. needs-triage Indicates an issue or PR lacks a label and requires one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carlory @kerthcet @InftyAI-Agent
0