feat(cli): allow model passed by OpenAI client to be used in the served Mellea program - #1512
feat(cli): allow model passed by OpenAI client to be used in the served Mellea program#1512markstur wants to merge 2 commits into
Conversation
… functions The current model_options are filtered so that they can be used by backends. Model ID is one of the things explicitly removed so that the Mellea program has control to select a model. In some cases, it may be appropriate for the Mellea program to use the client provided model ID (i.e. use that to decide the ultimate LLM to finish the query). If we simply stop filtering model, then serve programs would need to pop() it from model_options when it is not wanted. Better to leave model_options as-is. Instead of simply adding a model parameter, here we provide client_options. This allows model to be used and also access to the original client options which should have many future uses. This does not include http things like auth. For a serve() function to get client_options, it simply needs to be added to the signature. Existing serve() functions are not affected. Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com> Assisted-by: IBM Bob Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com>
Signed-off-by: Mark Sturdevant <mark.sturdevant@ibm.com> Assisted-by: IBM Bob
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
Well-scoped, backwards-compatible feature. CI green, 52 tests pass, ruff/mypy clean.
3 suggestions before merge:
- README/docstring claim needs tightening (see inline comment)
- Dead code in example (_ollama_host)
- Test coverage gaps for async + extra passthrough
Overall: APPROVE — nothing blocks, just pre-merge tidying.
| Passed to serve() as client_options when the function declares that | ||
| parameter, giving it access to every field the client sent (including | ||
| model, user, n, and anything else) without those values leaking into | ||
| the backend generation parameters in model_options. |
There was a problem hiding this comment.
This docstring says client_options keeps fields like model out of model_options. That holds for the named fields in the exclusion set (model, user, n), but ChatCompletionRequest has extra: allow so arbitrary client-sent keys reach the backend via _build_model_options too — its exclusion list is allowlist-style, not blocklist-style.
Empirical: sending {"custom_field": "x"} lands custom_field in both model_options and client_options. If the intent is "the named routing fields are separated", tighten wording. If the intent is all extras stay out of model_options, the exclusion logic needs inverting.
| from mellea.core import ModelOutputThunk | ||
| from mellea.serve import ChatMessage | ||
|
|
||
| _ollama_host = os.environ.get("OLLAMA_HOST", "localhost:11434") |
There was a problem hiding this comment.
Dead code: _ollama_host is computed from OLLAMA_HOST but never used — start_session(model_id=chosen_model) reads it internally via the backend. Removing these 3 lines also orphans import os at line 27.
| assert errors[0]["type"] == "greater_than_equal" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_client_options_not_passed_when_not_declared( |
There was a problem hiding this comment.
Two test coverage gaps:
- No async test for
client_options(theis_asyncbranch at app.py:242 is untested with the new kwarg) - No test for
extra: allowpassthrough —TestBuildClientOptionscoversmodel,user, params,exclude_none, but nothing for unknown extra keys
AngeloDanducci
left a comment
AngeloDanducci
left a comment
There was a problem hiding this comment.
Took a look, LGTM pending Nigel's nits.
3 participants
Pull Request
Issue
Fixes #1511
Description
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.