Skip to content

Wire model-config-datasource into the runner and helm chart.#175

Open
Mohammad-nassar10 wants to merge 2 commits into
llm-d:mainfrom
Mohammad-nassar10:modelconfig-wire
Open

Wire model-config-datasource into the runner and helm chart.#175
Mohammad-nassar10 wants to merge 2 commits into
llm-d:mainfrom
Mohammad-nassar10:modelconfig-wire

Conversation

@Mohammad-nassar10

Copy link
Copy Markdown
Contributor

What type of PR is this?
/kind feature

What this PR does / why we need it:
Register model-config-datasource plugin in the runner and expose a way to ship the JSON file through the chart, so users use it to seed candidate models.

Which issue(s) this PR fixes:

Fixes #

Release note (write NONE if no user-facing change):

NONE

Signed-off-by: Mohammad <mohammad.nassar@ibm.com>
@github-actions github-actions Bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 17, 2026
Comment on lines +29 to +35
{{- if .Values.payloadProcessor.listModels }}
{{- $models := list }}
{{- range .Values.payloadProcessor.listModels }}
{{- $models = append $models (dict "name" .) }}
{{- end }}
models.json: {{ dict "models" $models | toJson | quote }}
{{- end }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it's not clear what is listModels from this code.
can you please add the relevant documentation to the readme.MD inside the helm chart dir?
the optional flags should be documentation so whoever uses the helm can configure IPP easily.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, added, thanks!

Signed-off-by: Mohammad <mohammad.nassar@ibm.com>
@github-actions github-actions Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 17, 2026

@nirrozenbaum nirrozenbaum left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Mohammad-nassar10 thanks for adding the doc.
now that I understand what was the intention in the PR - this looks somehow very limiting - the list of models is static, there is no way to add a model dynamically (nor remove). it seems like an easy way for a PoC but not the proper way moving forward.

the payload processing config is intended to be used for plugins configuration.
seems like this PR is making the config define the source of truth of available models (this is not a plugin, although one could argue data collector is).

If we take it to EPP's world to compare just to make the point -
think how it would look like if one would have to configure the available endpoints of EPP through the plugins config file. this sounds incorrect to me.

alternative direction that could work:
add a configmap that stores models. configmap could also add metadata for every model entry (e.g., like its cost per 1M tokens). add a controller that watches the "modelsList" configmap(s) and fills that data into the datastore.

@shmuelk

shmuelk commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

@nirrozenbaum wrote:

the list of models is static, there is no way to add a model dynamically (nor remove). it seems like an easy way for a PoC but not the proper way moving forward.

The updates to the README.md state:

payloadProcessor.listModels is a list of model names that the IPP should be aware of. When set, the chart renders a models.json file into the ConfigMap, which is mounted at /config/models.json inside the pod. The model-config-datasource plugin reads this file to populate the IPP datastore and watches it for live updates on ConfigMap remounts.

That is not part of the IPP's configuration ConfigMap. It is a separate ConfigMap. If and how that ConfigMap is checked for changes is a different thing, which should be handled separately from this PR.

@ronenkat

Copy link
Copy Markdown
Contributor

@Mohammad-nassar10 thanks for adding the doc. now that I understand what was the intention in the PR - this looks somehow very limiting - the list of models is static, there is no way to add a model dynamically (nor remove). it seems like an easy way for a PoC but not the proper way moving forward.

the payload processing config is intended to be used for plugins configuration. seems like this PR is making the config define the source of truth of available models (this is not a plugin, although one could argue data collector is).

If we take it to EPP's world to compare just to make the point - think how it would look like if one would have to configure the available endpoints of EPP through the plugins config file. this sounds incorrect to me.

alternative direction that could work: add a configmap that stores models. configmap could also add metadata for every model entry (e.g., like its cost per 1M tokens). add a controller that watches the "modelsList" configmap(s) and fills that data into the datastore.

We already have a modelconfigcollector data source that reads a models-config.json file when ever it changes.
This PR enables IPP to run with a pre-defined list of models.
There could be multiple ways to configure IPP with models, this is just the first simple option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants