Skip to content

Remove model from the Conversation building block and promote to a runtime-provided argument - #108

Open
WhitWaldo wants to merge 2 commits into
dapr:mainfrom
WhitWaldo:conversation-models
Open

Remove model from the Conversation building block and promote to a runtime-provided argument#108
WhitWaldo wants to merge 2 commits into
dapr:mainfrom
WhitWaldo:conversation-models

Conversation

@WhitWaldo

@WhitWaldo WhitWaldo commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Please see the proposal for details

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo WhitWaldo self-assigned this Jun 19, 2026
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@acroca

acroca commented Jul 2, 2026

Copy link
Copy Markdown
Member

I agree with this change, the model belongs on the request.

My first reaction was that this changes the meaning of the Conversation component: without the model, it stops being "a conversation configuration" and becomes more of an "LLM provider" component (endpoint + credentials). But looking at the current implementation, that shift already happened, temperature, tools, response format, etc. are all on the request already. The model was the last piece left in the component, so this proposal is just finishing what was already started.

One thing I'd change: I think the allowlist mentioned in Future Work should ship with this proposal, not later. Today the component pins the model, so an admin controls exactly what gets used. With this change, any caller can pick any model the API key can reach, including very expensive ones. An optional allowlist on the component (default: all models allowed) would keep that control available without getting in anyone's way. Since it's optional it doesn't add friction for simple setups, but it lets component admins keep costs under control from day one instead of waiting for a follow-up proposal.

@WhitWaldo

WhitWaldo commented Jul 3, 2026

Copy link
Copy Markdown
Contributor Author

@acroca I certainly think it would be valuable to ship something like the allowlist, but I know work was done on the allow/deny list concept for workflows this last release and I don't know where any of that landed, so I've sort of left that as an open-ended thing for runtime to tackle.

But I'd certainly like to see a proposal for it submitted alongside this to have both sets of features shipped in the same release. It doesn't make a lot of sense to rewrite the wheel if there's a suitable one already that can be slotted in here, but I just don't know enough about what already exists to speak to that or not.

@mikeee

mikeee commented Jul 7, 2026

Copy link
Copy Markdown
Member

I'm a +1 on the proposal as it currently is, agreeing strongly with Albert that we bring governance into the initial scope.

https://github.com/dapr/dapr/blob/master/pkg/acl/workflow/policy.go

This is what landed on the last release Whit ^

@sicoyle sicoyle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

few comments so far from me pls, but overall am aligned with this change. It aligns with us using openai's api for inspiration too:
https://github.com/openai/openai-go/blob/main/chatcompletion.go#L192

Comment on lines +132 to +133
### Interaction with `context_id`
With model now per-request, a follow-up request that reuses an existing `context_id` may carry a different model than the request that opened the context. Whether switching models within a single context is meaningful is provider-defined; the runtime makes no guarantee here, consistent with the Crypto stance above. The model binds per request.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pls double check but context_id is not used anywhere IIRC

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.

This might be a bleed-over from the MAF integration. I'll follow up.

## Backwards Compatibility
This is a breaking change, but the Conversation block is in alpha, so this is the time to do this.

## Acceptance Criteria

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dapr agents and every other agent framework leveraging this api will need to expose a new parameter upon agent instantiation to support this

## Backwards Compatibility
This is a breaking change, but the Conversation block is in alpha, so this is the time to do this.

## Acceptance Criteria

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

contrib will need a bunch of updates to rm defaulting and removing this field and properly propagating this new req field over

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.

From a prototype level, I think it's just a refactoring to use the model from the request instead of the configuration component, except with regards to the DeepSeek component which doesn't expose a model at all (though their API allows more than one since the component was authored). But DeepSeek needs updating this month anyway since it defaults to "deepseek-chat" and that's being deprecated in their API later this month (7/24/26).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants