Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test #21

Conversation

LittleLittleCloud
Copy link
Collaborator

Why are these changes needed?

Related issue number

Checks

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2023

Codecov Report

Merging #21 (0a42469) into main (8e9cda7) will increase coverage by 3.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   32.12%   35.16%   +3.04%     
==========================================
  Files          16       17       +1     
  Lines        1958     1965       +7     
  Branches      432      432              
==========================================
+ Hits          629      691      +62     
+ Misses       1276     1223      -53     
+ Partials       53       51       -2     
Flag Coverage Δ
unittests 35.11% <ø> (+2.98%) ⬆️

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

see 4 files with indirect coverage changes

@LittleLittleCloud LittleLittleCloud mentioned this pull request Sep 27, 2023
3 tasks
@qingyun-wu
Copy link
Contributor

Can you by the way, remove this line

# %pip install --quiet flaml"[autogen]"~=2.1.0

from the first code block of this notebook: https://github.com/microsoft/autogen/blob/main/notebook/agentchat_teaching.ipynb

@sonichi
Copy link
Contributor

sonichi commented Sep 27, 2023

Can you by the way, remove this line

# %pip install --quiet flaml"[autogen]"~=2.1.0

from the first code block of this notebook: https://github.com/microsoft/autogen/blob/main/notebook/agentchat_teaching.ipynb

I'll make that change.

@sonichi
Copy link
Contributor

sonichi commented Sep 27, 2023

This PR doesn't trigger OpenAI test. Do you know how to trigger it?

@LittleLittleCloud
Copy link
Collaborator Author

pull_request_target event runs in the context of the base repository of the pull request, rather than in the merge commit. This is designed specifically for the purpose of providing a safe execution environment for workflows that originate from forks.

That's why, you probably need to merge it to main so the rest of PR will be able to trigger that workflow

That actually reminds me that I forget to checkout to the merge commit to run the test. Can you not merge this PR until I add that checkout step?

@sonichi
Copy link
Contributor

sonichi commented Sep 27, 2023

pull_request_target event runs in the context of the base repository of the pull request, rather than in the merge commit. This is designed specifically for the purpose of providing a safe execution environment for workflows that originate from forks.

That's why, you probably need to merge it to main so the rest of PR will be able to trigger that workflow

That actually reminds me that I forget to checkout to the merge commit to run the test. Can you not merge this PR until I add that checkout step?

The PR should trigger the test based on the rules. Do you know why it doesn't trigger? I'd like to test it before merging.

@LittleLittleCloud
Copy link
Collaborator Author

@sonichi

The workflow with pull_request_target needs to be merged to the base repo for testing. It won't be triggered by a pull request.

This is because pull_request_target event runs in the context of the base repository of the pull request, rather than in the merge commit. For example, the base repository of this pull request is microsoft:main, and there's no code change detected. Therefore, the openai workflow won't be triggerd.

In order to run the test, we need to checkout to the merge commit so the change from pull request will be applied. I haven't add the step to checkout to merge commit in openai workflow yet, so please don't merge this PR

@qingyun-wu qingyun-wu self-requested a review October 1, 2023 03:05
@sonichi
Copy link
Contributor

sonichi commented Oct 1, 2023

Could you make this a draft until it's ready?

@LittleLittleCloud LittleLittleCloud marked this pull request as draft October 1, 2023 07:21
@LittleLittleCloud LittleLittleCloud changed the title wip - update OpenAI workflow test Oct 2, 2023
@LittleLittleCloud LittleLittleCloud changed the base branch from main to u/xiaoyun/openai October 2, 2023 19:31
@LittleLittleCloud LittleLittleCloud marked this pull request as ready for review October 2, 2023 19:32
randombet pushed a commit to randombet/autogen that referenced this pull request Sep 26, 2024
* update

* update

* Added exceptions for non-openai clients into clients.py, corrected cohere stop error, removed try/except for Ollama

---------

Co-authored-by: Yiran Wu <[email protected]>
Co-authored-by: Qingyun Wu <[email protected]>
Co-authored-by: Chi Wang <[email protected]>
Co-authored-by: Mark Sze <[email protected]>
jackgerrits pushed a commit that referenced this pull request Oct 2, 2024
Added FAQ.md as placeholder
jackgerrits added a commit that referenced this pull request Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants