fix: remove repeated completion assignment in llm.py

[WannaTen]

End-user friendly description of the problem this fixes or functionality that this introduces

• remove duplicate code in llm.py

---

Give a summary of what the PR does, explaining any non-trivial design decisions

This duplicate code is introduced here: https://github.com/All-Hands-AI/OpenHands/pull/4722/files#diff-b46ec9dd4b319d75809520e34b2559fd2ce46ca4895709eae9312aa04a8fa9a4L151, I don't know what its purpose is, so send a pr to delete.

---

Link of any specific issues this addresses

[WannaTen]

@rbren Hi rb, can you have a look on this pr?

[enyst]

This assignment is not duplicate, I don't think? If you feel it is, could you point out where is the other?

The logging lines are duplicate, and it's nice to clean them out, thanks!

[WannaTen]

https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/llm/llm.py#L111
https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/llm/llm.py#L132
this two lines do the same assignment, I dont know what the point is, maybe I should assign an issue first?

[enyst]

Oh, you're right, this is so weird. I have a guess... the init model info call is necessary for the variables assigned in the partial, some wires must have got crossed because of that.

There's no need for an issue, sorry.

[enyst]

What do you think about this hypothesis - could you please check the order, e.g. that we need initialize model info at the right time?

[WannaTen]

What do you think about this hypothesis - could you please check the order, e.g. that we need initialize model info at the right time?

I checked the init phase in self.init_model_info(), saw the self.config.max_output_tokens is assigned a new value in init_model_info after assigned to partial. We need reorder the init_model_info and partial

BTW, I don't think partial would change any variable which is outside of partial code scope, so the first partial is unused actually.
We can ask @rbren for confirmation, he submitted this commit.

[WannaTen]

@enyst I reordered init_model_info and partial, make self.max_output_token assigned before using it in partial.

Besides, I wondering whether we need to assign a default number (i.e. 4096) to max_output_token in init_model_info() if it is none .
In litellm completion, if max_token is none, it means infinity output tokens. But in openhands, if user does not set a token number, and if litellm does not set a default max_token in model_info neither, openhands just set it to 4096, this breaks the hidden rules: user does not care, litellm does not care, but oh care. if we really care, why we set it to None in core/llm_config.py, it doesn't make sense.

[enyst]

Thank you! This looks to me like a funky copy paste, thanks a lot for handling it!

[enyst]

Besides, I wondering whether we need to assign a default number (i.e. 4096) to max_output_token in init_model_info() if it is none .
In litellm completion, if max_token is none, it means infinity output tokens. But in openhands, if user does not set a token number, and if litellm does not set a default max_token in model_info neither, openhands just set it to 4096, this breaks the hidden rules: user does not care, litellm does not care, but oh care. if we really care, why we set it to None in core/llm_config.py, it doesn't make sense.

That's a good question. At the time of setting the value in the dataclass (None) we don't know if liteLLM will give us something... but I do agree it's weird.

I'd suggest it doesn't need to be part of this PR, but if you want to propose a PR changing that, we can look into it.

[WannaTen]

That's a good question. At the time of setting the value in the dataclass (None) we don't know if liteLLM will give us something... but I do agree it's weird.

I'd suggest it doesn't need to be part of this PR, but if you want to propose a PR changing that, we can look into it.

Thanks a lot, I will send another PR to discuss parameter settings in LLM configure