Skip to content

Conversation

@bitnom
Copy link
Contributor

@bitnom bitnom commented Jan 2, 2024

Why are these changes needed?

Currently, the user's setting of stream: True is disregarded (Set to False) whenever function calling is used. We should honor the user's decision, and pave the way for incremental response processing and chunked callback functionality.

Related issue number

Resolves reviews of #786 , making the work done in #597 more complete.

Closes #785.

Let's also ping #831 since it was linked to #786 for some reason.

Checks

@bitnom
Copy link
Contributor Author

bitnom commented Jan 2, 2024

@microsoft-github-policy-service agree

@davorrunje
Copy link
Contributor

HI! Could you please include a test for it?

@bitnom
Copy link
Contributor Author

bitnom commented Jan 2, 2024

HI! Could you please include a test for it?

Thanks. I'm pretty sure this PR doesn't warrant a new test. No input or return schemas have been modified. It's just supplying what's already expected via the existing pydantic model, which was already being used for the return data, and should already have coverage.

I could be mistaken but if so, I'm not yet seeing the test-case.

@codecov-commenter
Copy link

codecov-commenter commented Jan 2, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 51.19%. Comparing base (3680197) to head (379f1c2).
Report is 2064 commits behind head on main.

Files with missing lines Patch % Lines
autogen/oai/client.py 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1118       +/-   ##
===========================================
+ Coverage   31.92%   51.19%   +19.26%     
===========================================
  Files          29       29               
  Lines        4097     4112       +15     
  Branches      955     1012       +57     
===========================================
+ Hits         1308     2105      +797     
+ Misses       2695     1806      -889     
- Partials       94      201      +107     
Flag Coverage Δ
unittests 51.09% <87.50%> (+19.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davorrunje
Copy link
Contributor

Two things:

  1. Please install pre-commit hook and run it on all files:
pre-commit install
pre-commit run --all-files

That should reformat the source using black. Otherwise, the code formatting check will fail (https://github.com/microsoft/autogen/actions/runs/7384108079/job/20093405650?pr=1118).

  1. Please run the existing tests, but remove .cache before:
rm -rf .cache
coverage run -a -m pytest test/oai/test_client_stream.py

You should get the following error:

====================================================== test session starts ======================================================
platform linux -- Python 3.10.12, pytest-7.4.3, pluggy-1.3.0
rootdir: /workspaces/autogen
configfile: pyproject.toml
plugins: asyncio-0.23.2, anyio-4.1.0
asyncio: mode=strict
collected 4 items                                                                                                               

test/oai/test_client_stream.py ...F                                                                                       [100%]

=========================================================== FAILURES ============================================================
____________________________________________________ test_completion_stream _____________________________________________________

    @pytest.mark.skipif(skip, reason="openai>=1 not installed")
    def test_completion_stream():
        config_list = config_list_openai_aoai(KEY_LOC)
        client = OpenAIWrapper(config_list=config_list)
>       response = client.create(prompt="1+1=", model="gpt-3.5-turbo-instruct", stream=True)

test/oai/test_client_stream.py:79: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
autogen/oai/client.py:272: in create
    response.cost = self.cost(response)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <autogen.oai.client.OpenAIWrapper object at 0x7f8274375120>, response = <openai.Stream object at 0x7f8275026d70>

    def cost(self, response: Union[ChatCompletion, Completion]) -> float:
        """Calculate the cost of the response."""
>       model = response.model
E       AttributeError: 'Stream' object has no attribute 'model'

autogen/oai/client.py:468: AttributeError
==================================================== short test summary info ====================================================
FAILED test/oai/test_client_stream.py::test_completion_stream - AttributeError: 'Stream' object has no attribute 'model'
================================================== 1 failed, 3 passed in 2.93s ==================================================

Copy link
Contributor

@davorrunje davorrunje left a comment

Choose a reason for hiding this comment

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

Please fix black formatting and failed test as described in the comment.

@ekzhu
Copy link
Contributor

ekzhu commented Jan 7, 2024

@sonichi run openai tests

@davorrunje
Copy link
Contributor

@sonichi I made local changes to use tool_calls instead of function_call, but cannot test it properly before #974 is merged. I propose to wait for #974 and then test this properly before merging it.

That PR was merged, so I updated from main and passed the tests. Manually firing up streaming and non-streaming agents worked as well.

After this, I have 2 PRs incoming which will resolve some dependent issues (mostly feature requests), and I'll replace all token counters in the repo with tiktoken (or similar/faster based on tests).

@bitnom as @sonichi noticed, choice.delta.function_call is deprecated and we should use choice.delta.tool_calls instead (https://platform.openai.com/docs/api-reference/chat/streaming). If tests are passing, we are probably not triggering tool_calls but a deprecated function_call. This could be fixed in this PR or a new one. I can do it tomorrow in either case.

@ekzhu
Copy link
Contributor

ekzhu commented Jan 7, 2024

Thanks @davorrunje for pointing this out I missed it. Let's do it in this PR as it is the most relevant.

@bitnom
Copy link
Contributor Author

bitnom commented Jan 7, 2024

@sonichi I made local changes to use tool_calls instead of function_call, but cannot test it properly before #974 is merged. I propose to wait for #974 and then test this properly before merging it.

That PR was merged, so I updated from main and passed the tests. Manually firing up streaming and non-streaming agents worked as well.
After this, I have 2 PRs incoming which will resolve some dependent issues (mostly feature requests), and I'll replace all token counters in the repo with tiktoken (or similar/faster based on tests).

@bitnom as @sonichi noticed, choice.delta.function_call is deprecated and we should use choice.delta.tool_calls instead (https://platform.openai.com/docs/api-reference/chat/streaming). If tests are passing, we are probably not triggering tool_calls but a deprecated function_call. This could be fixed in this PR or a new one. I can do it tomorrow in either case.

Thanks @davorrunje for pointing this out I missed it. Let's do it in this PR as it is the most relevant.

This is correct. My initial reaction was to want to do it separately since autogen was founded on the deprecated methods. I know there have already been commits merged for using the tools stuff though.

I have yet to come to terms with this deprecation myself. I'll read up on it. If someone can zip through it ahead of me, please feel free to go ahead with it. I have some tasks I must complete before I can get to it.

@sonichi
Copy link
Contributor

sonichi commented Jan 8, 2024

Perhaps we can merge this PR first and add support for tool call in a different PR.

@sonichi sonichi added this pull request to the merge queue Jan 8, 2024
Merged via the queue into microsoft:main with commit 78a2d84 Jan 8, 2024
@tyler-suard-parker
Copy link
Contributor

@bitnom I need streaming with functions calling too. Is there anything I can do to help?

@davorrunje
Copy link
Contributor

@bitnom I need streaming with functions calling too. Is there anything I can do to help?

This is merged, but supports only deprecated function calls. I am working to support tool calls that replaced function calls. Should be finished this week.

@tyler-suard-parker
Copy link
Contributor

@davorrunje Ok, thank you.

@tyler-suard-parker
Copy link
Contributor

I tried downloading and using this updated repo, and it is still not streaming.

@davorrunje
Copy link
Contributor

@tyler-suard-parker I made #1184 which should fix it. You could try it out by installing pyautogen from the branch.

@tyler-suard-parker
Copy link
Contributor

@davorrunje thank you, I really appreciate your help. I will try it now.

whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* update colab link

* typo

* upload file instruction
whiskyboy pushed a commit to whiskyboy/autogen that referenced this pull request Apr 17, 2024
* Handle streamed function calls

* apply black formatting

* rm unnecessary stdout print

* bug fix

---------

Co-authored-by: Davor Runje <davor@airt.ai>
Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tool-usage suggestion and execution of function/tool call

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streaming is disabled for all conversation when agents are using functions

7 participants