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

Add "py" as lang in conversable agent (#1062) #2144

Merged
merged 10 commits into from
Apr 12, 2024
Merged

Conversation

JoshTrim
Copy link
Collaborator

@JoshTrim JoshTrim commented Mar 25, 2024

Why are these changes needed?

Adds "py" string to conditional in conversable_agent, in order to execute code blocks.

Related issue number

Closes #1062

Checks

Please let me know if there are any issues with this PR, still new to OS development.

Copy link
Collaborator

@ekzhu ekzhu left a comment

Choose a reason for hiding this comment

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

  1. Update the function here also:
    def _cmd(lang: str) -> str:
    to handle cases when lang is py and Python, converting them to python.
  2. Update unit tests to handle py, Python, and python: https://github.com/microsoft/autogen/blob/main/test/coding/test_commandline_code_executor.py#L66

@ekzhu ekzhu added the code-execution execute generated code label Mar 25, 2024
@JoshTrim
Copy link
Collaborator Author

@microsoft-github-policy-service agree

@JoshTrim
Copy link
Collaborator Author

@ekzhu Thankyou for the pointers. Second commit is to revert an accidental import.

It looks good now. In addition to your suggestion I had to add the condition here, in order to pass tests: 99a2a11#diff-7e1e6bfc49ab6dff4b52ba5677edcda6aff4ae436001c46cecd45847d4d71caeR119-R120

@JoshTrim JoshTrim requested a review from ekzhu March 27, 2024 08:38
@BeibinLi
Copy link
Collaborator

Thanks~

This PR is also related to the issue #2096 I made updates to the Gemini branch but not the main branch. This update will be good for many other models.

@sonichi
Copy link
Contributor

sonichi commented Apr 10, 2024

@ekzhu this PR is blocked by your change request.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 39.07%. Comparing base (4a44093) to head (88b3a34).
Report is 9 commits behind head on main.

Files Patch % Lines
autogen/code_utils.py 0.00% 1 Missing and 1 partial ⚠️
autogen/agentchat/conversable_agent.py 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2144      +/-   ##
==========================================
+ Coverage   38.14%   39.07%   +0.92%     
==========================================
  Files          78       78              
  Lines        7865     7883      +18     
  Branches     1683     1825     +142     
==========================================
+ Hits         3000     3080      +80     
+ Misses       4615     4539      -76     
- Partials      250      264      +14     
Flag Coverage Δ
unittests 39.02% <40.00%> (+0.88%) ⬆️

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.

Copy link
Collaborator

@ekzhu ekzhu left a comment

Choose a reason for hiding this comment

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

Thanks. This can be improved by using parametetrized pytest decorators. You can create a separate PR for this.

# content of test_math.py
import pytest

def calculate_square(number):
    return number ** 2

@pytest.mark.parametrize("input_number, expected_result", [
    (2, 4),  # Test case 1: Input 2, expect 4
    (3, 9),  # Test case 2: Input 3, expect 9
    (0, 0),  # Test case 3: Input 0, expect 0
    (-5, 25)  # Test case 4: Input -5, expect 25
])
def test_calculate_square(input_number, expected_result):
    result = calculate_square(input_number)
    assert result == expected_result

autogen/code_utils.py Outdated Show resolved Hide resolved
JoshTrim and others added 3 commits April 11, 2024 21:42
Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
)

Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>
@ekzhu
Copy link
Collaborator

ekzhu commented Apr 12, 2024

Run pre-commit run --all-files to spot and fix formatting error.

@JoshTrim
Copy link
Collaborator Author

Run pre-commit run --all-files to spot and fix formatting error.

Done.

@ekzhu ekzhu enabled auto-merge April 12, 2024 14:54
@ekzhu ekzhu added this pull request to the merge queue Apr 12, 2024
Merged via the queue into microsoft:main with commit d473dee Apr 12, 2024
60 of 72 checks passed
@JoshTrim JoshTrim deleted the handle-py branch April 12, 2024 23:13
jayralencar pushed a commit to jayralencar/autogen that referenced this pull request May 28, 2024
* Add "py" as lang in conversable agent (microsoft#1062)

* Add conditions to allow for python executable variants (microsoft#1062)

* reverted import (microsoft#1062)

* Parameterized tests, moved Python variants to a constant (microsoft#1062)

* Moved Python variants to a constant (microsoft#1062)

* Update autogen/code_utils.py (microsoft#1062)

Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>

* Update autogen/coding/local_commandline_code_executor.py (microsoft#1062)

Co-authored-by: Eric Zhu <ekzhu@users.noreply.github.com>

* Added PYTHON_VARIANTS as imported constant (microsoft#1062)

* ran pre-commit-check  (microsoft#1062)

---------

Co-authored-by: Chi Wang <wang.chi@microsoft.com>
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
code-execution execute generated code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] handles py code?
6 participants