-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Refactor evaluate ast to improve readability #625
Refactor evaluate ast to improve readability #625
Conversation
raise InterpreterError(f"{expression.__class__.__name__} is not supported.") | ||
common_params = (state, static_tools, custom_tools, authorized_imports) | ||
evaluate_ast_partial = partial(evaluate_ast, state=state, static_tools=static_tools, custom_tools=custom_tools, authorized_imports=authorized_imports) | ||
match expression: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This requires python >= 3.10. Wonder if maintainers are ok with this 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it is a fair concern, I checked in the project's pyproject.toml
and the Python version is requires-python = ">=3.10"
so it should be fine, but if this refactor is the only thing preventing the project from handling python 3.9 it's a tough sell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, no idea. They don't use tox here to test against different versions. I guess we need to add that. Maybe that ship has already sailed and adding matching is fine :)
3ce7c97
to
093a54f
Compare
@albertvillanova I noticed this change was not getting much traction so I changed it to its most basic form. Let me know if that works for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, it seems you didn't run the quality checks before pushing your PR.
Here you can find the contributing guideline: https://github.com/huggingface/smolagents/blob/main/CONTRIBUTING.md
Hey @albertvillanova thanks for taking the time to look at this, I did run the quality checks, but not with the same command as the CI, hence the discrepancy (I did This is fixed now, is there an issue tracking the update of the Makefile/pyproject to add a dependency on uv/update the targets? I can pick that up if you guys are a bit tight on bandwidth. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
This PR does 3 main things:
common_params
variable for use instead of retypingstate, static_tools, custom_tools, authorized_imports
every timeelif
machine with amatch
statementevaluate_ast_partial
, a partial evaluation ofevaluate_ast
allowing it to be used withmap
I'll concede the later 2 are more of an opinionated choice than a straight improvement to readability, let me know if you think so too and I'll discard them.