Updated llmclient with the newest ldp implementations

[maykcaldas]

LDP had updated since llmclient project started. Mainly, ldp.llms was changed. This PR brings the changes from chat.py and test_llms.py to llmclient.

This PR is blocking Future-House/ldp#175

[jamesbraza]

Can we get some unit testing? I think my change had unit tests too

[jamesbraza]

To share, I used this import to work around us having to manage jsonschema in our own deps, instead we let litellm internals manage the implementation

[jamesbraza]

@mskarlin had a question in the original PR here: Future-House/ldp#165 (comment), which was if we can have a validation of the model name here (more or less). Since making that change, I learned of litellm.supports_response_schema("gpt-4o-2024-11-20", None) which can enable that check. What do you think of:

1. Moving chat_kwargs = self.config | chat_kwargs above this part of the code
2. Doing a brief validation here such as:

model_name: str = chat_kwargs.get("model", "")
if not litellm.supports_response_schema(model_name, None):
    raise ValueError(f"Model {model_name} does not support JSON schema.")

Or, do you think we should just pass through to litellm and either:

• If litellm has validations, let them check
• Otherwise let the LLM API blow up with something like a 400

I am sort of in favor of the latter, which is our code here not doing validations, instead sort of "trusting the process" and letting callers get blown up with a trace terminating below llmclient.

What do you think?

[maykcaldas]

I think that allowing it to pass and blowing on the LLM API will give us more issues in the future. Because we don't know how litellm treats this, the error can be uninformative and the user will open issues here.

Since litellm already maintains a list of models that supports this schema and we can use the supports_response_schema, wouldn't it be useful to add a brief check so we can return an informative error message?

[jamesbraza]

I guess there could be a situation where litellm is wrong and/or out-of-date, and a validation may block us.

However, a validation also does better define the system, and add confidence in it.

I can see arguments for both sides, defer to you here

[maykcaldas]

Since we are using litellm as the main framework, I think it might be ok to trust it?
I added the check. Let me know what you think.

We can also remove it later if it causes any problems

[maykcaldas]

This was moved above to validate if the model supports the schema