Skip to content

Add math test#555

Closed
yiranwu0 wants to merge 18 commits into
mainfrom
mathtest
Closed

Add math test#555
yiranwu0 wants to merge 18 commits into
mainfrom
mathtest

Conversation

@yiranwu0

@yiranwu0 yiranwu0 commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

Why are these changes needed?

  • Add Contrib test for math proxy agent.
  • remove [mathchat] dependency in build test flow.
  • Putting tests for different contrib agents in one job. Previous test fail will not block sequential tests.

Example that previous test fails doesn’t block sequential tests
https://github.com/microsoft/autogen/actions/runs/6759692725/job/18372611806

(Install packages and dependencies for RetrieveChat -> fails
Run “Install packages and dependencies for MathChat” -> success
Run “test MathChat” -> success
Result is still error
)

When previous dependency fails, corresponding testing will be disabled.
https://github.com/microsoft/autogen/actions/runs/6759701280/job/18372633270?pr=555
(Run “ Install packages and dependencies for RetrieveChat” -> fail
Run “”Install packages and dependencies for MathChat ->fail
skipping test MathChat)

Related issue number

Checks

@codecov-commenter

codecov-commenter commented Nov 5, 2023

Copy link
Copy Markdown

Codecov Report

Merging #555 (9735537) into main (fda7a39) will increase coverage by 4.88%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #555      +/-   ##
==========================================
+ Coverage   32.40%   37.29%   +4.88%     
==========================================
  Files          27       27              
  Lines        3357     3357              
  Branches      756      756              
==========================================
+ Hits         1088     1252     +164     
+ Misses       2173     1989     -184     
- Partials       96      116      +20     
Flag Coverage Δ
unittests 37.23% <ø> (+4.88%) ⬆️

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

see 3 files with indirect coverage changes

AZURE_OPENAI_API_BASE: ${{ secrets.AZURE_OPENAI_API_BASE }}
OAI_CONFIG_LIST: ${{ secrets.OAI_CONFIG_LIST }}
run: |
coverage run -a -m pytest test/agentchat/contrib/test_math_user_proxy_agent.py

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks like the output of coverage run will overwrite the output of the former step. In the end, only one step of coverage will be uploaded.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could we add different names to the output of different steps and combine them at the upload step?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The coverage run -a will append outputs to the same file(.coverage), so we are good with this. Not very sure but this is what I find.

Comment thread .github/workflows/contrib-tests.yml

jobs:
RetrieveChatTest:
OpenAI4ContribTests:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One problem with this approach is that the previous installed dependencies will remain in the later steps. That makes the test environment not clean for later steps. Is it possible to reset the environment for later steps?

@yiranwu0 yiranwu0 Nov 5, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I haven't found any good way to clear up the environment. A possible solution is to use virtual env, but different os needs different syntax, which makes it complex. The other way is just putting them in different jobs.

I guess there are two problems to consider here:

  1. Is it ok to NOT having a clean env? Will there be a case that user does pip install autogen[retrievechat, mathchat], and they expect things just work?
  2. I suggested that separating the jobs would be tedious to look in github actions, which is why I change this in the first place. But we might need to revisit the problem: do you think it is indeed a issue, or we can bear with it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's pretty important for the automatic jobs to test different contributed agents in different environments, each with just its own dependencies, whether this is done through separate virtual envs, different jobs, or some other mechanism. Wish I had more experience in setting up such tests.

@yiranwu0 yiranwu0 Nov 6, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

coverage run -a -m pytest test/test_retrieve_utils.py test/agentchat/contrib

will test all files in the contrib folder, while only dependencies for Retrievechat is installed. This will result in error in the future if a contrib agent requires other packages to be installed.
https://github.com/microsoft/autogen/blob/fda7a39dd9ede1c115d37c2a454f55b7f22c8193/.github/workflows/contrib-tests.yml#L52C38-L52C38

@yiranwu0 yiranwu0 mentioned this pull request Nov 25, 2023
3 tasks
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