microsoft / teams-ai

SDK focused on building AI based applications and extensions for Microsoft Teams and other Bot Framework channels
MIT License
349 stars 143 forks source link

[JS] feat: add Streaming support #1717

Closed Stevenic closed 1 week ago

Stevenic commented 3 weeks ago

Linked issues

closes: #1593

Details

Updated JS OpenAIModel class to support streaming and updated ActionPlanner to support streaming chunks to the client.

Change details

Attestation Checklist

Additional information

I started basic unit tests for the LLMClient class. These tests should be extended to provider better coverage. We should also add unit tests for the ActionPlanner class now that the TestModel class exists.

corinagum commented 2 weeks ago

@Stevenic could you mark the class/methods in LLMClient as deprecated? Notes indicating to use StreamingLLMClient instead would be great.

Stevenic commented 2 weeks ago

@Stevenic could you mark the class/methods in LLMClient as deprecated? Notes indicating to use StreamingLLMClient instead would be great.

It's not really deprecated though. You should just generally use the derived class. It will trigger warnings if I mark it as deprecated since it's a base class

corinagum commented 2 weeks ago

It's not really deprecated though. You should just generally use the derived class. It will trigger warnings if I mark it as deprecated since it's a base class

Okay so maybe marking it as deprecated isn't the right answer, but I find it confusing that the base class is only being used by one derived class. This is specifically done for backwards compatibility, correct? In that case there should be remarks to that effect, and what scenarios you would want to use the base class. Feel free to let me know if this is just misconception from my functions oriented brain, but personally the extra comments would be beneficial.

Stevenic commented 1 week ago

It's not really deprecated though. You should just generally use the derived class. It will trigger warnings if I mark it as deprecated since it's a base class

Okay so maybe marking it as deprecated isn't the right answer, but I find it confusing that the base class is only being used by one derived class. This is specifically done for backwards compatibility, correct? In that case there should be remarks to that effect, and what scenarios you would want to use the base class. Feel free to let me know if this is just misconception from my functions oriented brain, but personally the extra comments would be beneficial.

I can merge the two classes