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

Improved query, print & exception handling in REPL Tool #4997

Merged
merged 7 commits into from
May 22, 2023

Conversation

svdeepak99
Copy link
Contributor

Update to pull request #3215

Summary:

  1. Improved the sanitization of query (using regex), by removing python command (since gpt-3.5-turbo sometimes assumes python console as a terminal, and runs python command first which causes error). Also sometimes 1 line python codes contain single backticks.
  2. Added 7 new test cases.

For more details, view the previous pull request.

@svdeepak99
Copy link
Contributor Author

@vowelparrot kindly check this pull request.

@dev2049 dev2049 requested a review from vowelparrot May 19, 2023 21:31
@svdeepak99
Copy link
Contributor Author

Made a small change to make sure backticks(`) & python are not removed, if they are present inside of a code (such as print statements), using regex. Also added a test case to verify the same. The code is ready for review/merge now (@vowelparrot , @dev2049 ).

@svdeepak99
Copy link
Contributor Author

So this check fails because the python ast module was built for python 3.9 and above, which the tool fully relies on. I could still remove the PythonAstREPLTool() from the test cases (and instead just run the sanitize_input()) to make this check succeed. But that would cause the quality of the test cases to reduce. What would you suggest me to do now?

@svdeepak99
Copy link
Contributor Author

svdeepak99 commented May 22, 2023

I just found out that it is possible to solve this problem by modifying the following function in langchain/tools/python/tool.py as follows:

@root_validator(pre=True)
    def validate_python_version(cls, values: Dict) -> Dict:
        """Validate valid python version."""
        if sys.version_info < (3, 9):
            if sys.version_info[:2] == (3, 8):
                import astunparse
                ast.unparse = astunparse.unparse
            else:
                raise ValueError(
                    "This tool relies on Python 3.9 or higher "
                    "(as it uses new functionality in the `ast` module, "
                    f"you have Python version: {sys.version}"
                )
        return values

But this required me to pip install astunparse. Is it fine if I make this change & if yes, should I include this library in the poetry pyproject.toml file? @vowelparrot

Reference: plasma-umass/scalene#544 (comment)

@vowelparrot
Copy link
Contributor

vowelparrot commented May 22, 2023

We don't want to add additional depenencies unless absolutely necessary. If it works for python versions < 3.8, you can try importing and then throw a more helpful import error saying to either use python version > 3.9 or pip install the unparser.

For test cases, you could do

@pytest.mark.skipif(
    sys.version_info < (3, 9) reason="Python REPL not supported for python versions < 3.9"
)

To test the unparser specifically, we have "extended_tests" that are run with additional dependencies. (I think you can stack marks but if that's not the case we can remove the skipif and just return a pass if the version isn't 3.8 or lower)

@pytest.mark.skipif(sys.version_info >= (3, 0) reason
@pytest.mark.requires("astunparse")
def test_ast_repl_astunparse():
...

Then add the unparser to the extended_testing list in pyproject.toml

@svdeepak99
Copy link
Contributor Author

Ok sure, I can do that. Let me revert my last commit, and make the changes you mentioned. Thanks for your reply.

@svdeepak99
Copy link
Contributor Author

@vowelparrot I have skipped <3.9 python versions for the ast_repl test cases as you mentioned (thanks for helping me out). You can merge this PR now (it should pass all test cases).

@vowelparrot vowelparrot merged commit 49ca027 into langchain-ai:master May 22, 2023
@danielchalef danielchalef mentioned this pull request Jun 5, 2023
This was referenced Jun 25, 2023
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.

2 participants