feat: python tools requirement#1040
Conversation
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
The PR description has been updated. Please fill out the template for your PR to be reviewed. |
| result=False, | ||
| reason="Your code creates plots with pyplot but never calls `plt.savefig()` to save them.\n\n" | ||
| "Add this before your plotting code or at the end:\n" | ||
| " plt.savefig('{output_path}')\n" |
There was a problem hiding this comment.
I think this should match the approach in _make_output_artifacts_validator in the way it handles path/output_path.
I assume this was intended to be a f" string instead of a " string.
| "Fix this by adding to the top of your code:\n" | ||
| " import matplotlib\n" | ||
| " matplotlib.use('Agg')\n\n" | ||
| "Then replace `plt.show()` with `plt.savefig('{output_path}'); plt.close()`", |
There was a problem hiding this comment.
Same as below ie
I think this should match the approach in _make_output_artifacts_validator in the way it handles path/output_path.
I assume this was intended to be a f" string instead of a " string.
There was a problem hiding this comment.
Think this and the the other f" string still need to be changed - the strings starting on line 287 and 311.
My other review comments have been address 👍
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
jakelorocco
left a comment
There was a problem hiding this comment.
I think these new requirements also feel like they aren't building on pre-existing things in our library: Built on top of the existing uses_tool, tool_arg_validator, and PythonExecutionReq scaffolding. If they can't be, please detail why / improve the underlying implementations.
There was a problem hiding this comment.
We haven't been putting files directly into docs/examples. Please create a folder for this. I'm not sure what the folder should be; maybe it can go in the existing tools dir? I also see that docs/examples/as_generic_chat_history.py is in that same directory, can you please move it as well (either in this PR or a separate one).
| @@ -0,0 +1,174 @@ | |||
| # pytest: ollama, e2e, qualitative | |||
| """Granite 4.1 repairs the three canonical plotting failures with Python tool. | |||
There was a problem hiding this comment.
Since this is an example that lives in our main repo, the underlying model might change (especially since you are using start_session). Can you instead refer to the model or find some other wording to keep maintenance cost lower?
| Requirements: | ||
| - Use the python tool to execute your code | ||
| - Import numpy and matplotlib | ||
| - Generate x values from 0 to 2π | ||
| - Plot sin(x) against x | ||
| - Save the plot to the specified file path | ||
|
|
||
| Use the python tool with your complete code.""" | ||
| instruction = Instruction(description=description) |
There was a problem hiding this comment.
Is there a reason that these requirements aren't appended as actual "requirements" to the instruction but instead embedded in the description? If possible, I think the preferred way of writing this code would be:
m.instruct(<description>, <requirements>, <sampling_strategy>
We tend not to directly invoke the sampling strategy. Are you doing so just so that you can utilize the bundled python reqs from above?
| def get_unauthorized_imports( | ||
| code: str, allowed_imports: list[str] | None = None | ||
| ) -> list[str]: | ||
| r"""Extract unauthorized imports from Python code. |
There was a problem hiding this comment.
Does this need to be a raw string? It's just a docstring.
| FAILURE MATRIX — How each requirement catches the canonical plotting failures: | ||
|
|
||
| Scenario: Model generates plotting code with matplotlib | ||
|
|
||
| Attempt 1: No tool call | ||
| → MustInvokePythonTool fails | ||
| → Repair: "Call the `python` tool with your code" | ||
|
|
||
| Attempt 2: Tool called but no 'code' arg | ||
| → PythonToolHasCodeArg fails | ||
| → Repair: "The python tool requires a 'code' argument" | ||
|
|
||
| Attempt 3: Code has syntax error | ||
| → PythonCodeParses fails | ||
| → Repair: "Your code has a syntax error at line X: {error}" | ||
|
|
||
| Attempt 4: Code imports matplotlib (not in allowed_imports) | ||
| → PythonImportsAllowed fails | ||
| → Repair: "matplotlib is not allowed. Use only: {allowed_list}" | ||
|
|
||
| Attempt 5: Code uses plt.show() without headless backend | ||
| → MatplotlibHeadless fails | ||
| → Repair: "Add matplotlib.use('Agg') and replace plt.show() with plt.savefig(...)" | ||
|
|
||
| Attempt 6: Code has plt.plot() but no plt.savefig() | ||
| → PlotsAreSaved fails | ||
| → Repair: "Add plt.savefig('{output_path}') to save the plot" | ||
|
|
||
| Attempt 7: Code runs, but output file not created | ||
| → OutputArtifactsExist fails | ||
| → Repair: "File '{output_path}' was not created. Check plt.savefig() call" | ||
|
|
||
| Attempt 8: Success | ||
| → All requirements pass | ||
| → Result: plot file exists and is non-empty |
There was a problem hiding this comment.
I think you can drop this part of the docstring. The sampling strategies typically determine what actually happens.
| def _sets_headless_backend(code: str) -> bool: | ||
| """Check if code sets matplotlib to use a headless backend.""" | ||
| clean_code = _strip_comments(code) | ||
| headless_backends = ("Agg", "Svg", "Cairo", "PDF", "PS", "WebAgg", "nbAgg") | ||
| for backend in headless_backends: | ||
| if ( | ||
| f"matplotlib.use('{backend}')" in clean_code | ||
| or f'matplotlib.use("{backend}")' in clean_code | ||
| ): | ||
| return True | ||
| return False |
There was a problem hiding this comment.
Same as above. If we want some matplotlib specific requirements, then we should introduce structure into our requirements folder so that the specificity is easy to find and makes sense.
| def _uses_pyplot_plot(code: str) -> bool: | ||
| """Check if code calls pyplot plotting functions.""" | ||
| plot_functions = ( | ||
| "plt.plot", | ||
| "plt.bar", | ||
| "plt.scatter", | ||
| "plt.hist", | ||
| "plt.imshow", | ||
| "plt.figure", | ||
| "plt.subplot", | ||
| ".plot(", | ||
| ".bar(", | ||
| ".scatter(", | ||
| ".hist(", | ||
| ) | ||
| return any(func in code for func in plot_functions) | ||
|
|
||
|
|
||
| def _calls_savefig(code: str) -> bool: | ||
| """Check if code calls plt.savefig() or fig.savefig().""" | ||
| return "savefig" in code |
| class PythonToolRequirements: | ||
| """Pre-composed bundle of requirements for Python code generation via the tool. | ||
|
|
||
| This bundle validates the complete Python code generation flow: tool invocation, | ||
| syntax, imports, execution, and output. It's designed to work with repair loops | ||
| (SOFAI, MultiTurnStrategy) to iteratively fix common plotting failures. | ||
|
|
||
| Markers: | ||
| - **Deterministic** (unit-testable): tool invocation, syntax, imports, headless backend, | ||
| savefig presence, file existence, output limits | ||
| - **Qualitative** (needs model to evaluate): execution without error (captured via stderr) | ||
|
|
||
| Args: | ||
| output_path (str | None): Path where plots should be saved. If specified, enables | ||
| output artifact validation. Defaults to None. | ||
| allowed_imports (list[str] | None): Allowlist of importable top-level modules. | ||
| None (default) allows any import. Set to list like ["numpy", "matplotlib"] | ||
| to restrict imports. | ||
| output_limit_bytes (int): Maximum bytes of stdout/stderr allowed. Defaults to 50000. | ||
| check_output_artifacts (bool): If True, validate that output file exists and is | ||
| non-empty after execution. Defaults to True if output_path is specified. | ||
|
|
||
| Attributes: | ||
| requirements (list[Requirement]): The composed list of requirements, suitable | ||
| for use with sampling strategies. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| output_path: str | None = None, | ||
| allowed_imports: list[str] | None = None, | ||
| output_limit_bytes: int = 50_000, | ||
| check_output_artifacts: bool | None = None, | ||
| ): | ||
| """Initialize the Python tool requirements bundle.""" | ||
| self.output_path = output_path | ||
| self.allowed_imports = allowed_imports | ||
| self.output_limit_bytes = output_limit_bytes | ||
|
|
||
| # Auto-enable output artifact checking if output_path is specified | ||
| if check_output_artifacts is None: | ||
| check_output_artifacts = output_path is not None | ||
|
|
||
| self._check_output_artifacts = check_output_artifacts | ||
|
|
||
| self.requirements = self._build_requirements() | ||
|
|
||
| def _build_requirements(self) -> list[Requirement]: | ||
| """Build the list of requirements for this bundle.""" | ||
| reqs: list[Requirement] = [] | ||
|
|
||
| # Tool invocation requirements (deterministic) | ||
| reqs.append( | ||
| Requirement( | ||
| description="Use the python tool to execute code.", | ||
| validation_fn=_validate_python_tool_invoked, | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| reqs.append( | ||
| Requirement( | ||
| description="The python tool call must include a code argument.", | ||
| validation_fn=_validate_python_tool_has_code_arg, | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| # Code quality requirements (deterministic) | ||
| reqs.append( | ||
| Requirement( | ||
| description="The Python code must parse correctly.", | ||
| validation_fn=_make_code_parses_validator(), | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| # Import validation (deterministic) | ||
| if self.allowed_imports is not None: | ||
| reqs.append( | ||
| Requirement( | ||
| description=f"Imports must be from allowed list: {', '.join(self.allowed_imports)}", | ||
| validation_fn=_make_imports_allowed_validator(self.allowed_imports), | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| # Matplotlib-specific requirements (deterministic) | ||
| reqs.append( | ||
| Requirement( | ||
| description=( | ||
| "If using pyplot, must set headless backend and use savefig." | ||
| ), | ||
| validation_fn=_make_matplotlib_headless_validator(), | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| reqs.append( | ||
| Requirement( | ||
| description="If creating plots, must call savefig to save them.", | ||
| validation_fn=_make_plots_saved_validator(), | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| # Output artifact validation (deterministic, post-execution) | ||
| if self._check_output_artifacts and self.output_path: | ||
| reqs.append( | ||
| Requirement( | ||
| description=f"Output file must be created at {self.output_path}", | ||
| validation_fn=_make_output_artifacts_validator(self.output_path), | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| # Output limiting (deterministic) | ||
| reqs.append( | ||
| Requirement( | ||
| description=f"Output must not exceed {self.output_limit_bytes} bytes.", | ||
| validation_fn=_make_output_limit_validator(self.output_limit_bytes), | ||
| check_only=False, | ||
| ) | ||
| ) | ||
|
|
||
| return reqs | ||
|
|
||
| def __repr__(self) -> str: | ||
| """Return a developer-readable representation.""" | ||
| return ( | ||
| f"PythonToolRequirements(" | ||
| f"output_path={self.output_path!r}, " | ||
| f"allowed_imports={self.allowed_imports!r}, " | ||
| f"output_limit_bytes={self.output_limit_bytes}, " | ||
| f"requirements={len(self.requirements)} items" | ||
| f")" | ||
| ) |
There was a problem hiding this comment.
This is a completely new class / notation. I'm not sure we want to introduce something bundled like this. If anything, it should likely just be exported as a list of requirements. I don't know if it makes sense to introduce this grouping though. A lot of these python requirements are specific to matplotlib, not python in general.
There was a problem hiding this comment.
In general, I feel like this file has a lot going on. I think additional structure (either through folders or splitting up this file), would be helpful. If we need to make sub folders with their own helper function files, that seems reasonable to.
There was a problem hiding this comment.
I'm not sure this is the correct place for this file. Lets move it closer to python_tools or in the stdlib.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
I believe I addressed all comments so far. |
jakelorocco
left a comment
There was a problem hiding this comment.
A few more comments. I like how you handled the requirement bundling; I think that's a nice touch.
| Handles various matplotlib import styles and fallback to string matching. | ||
| """ | ||
| if _find_function_calls(code, ["matplotlib.use"]): | ||
| headless_backends = {"Agg", "Svg", "Cairo", "PDF", "PS", "WebAgg", "nbAgg"} |
There was a problem hiding this comment.
Can this be extracted as a constant? I imagine that we would always want to use the same set of headless backends no matter the context?
| except (SyntaxError, ValueError): | ||
| return _code_contains_strings( | ||
| code, [f"matplotlib.use('{b}')" for b in headless_backends] | ||
| ) |
There was a problem hiding this comment.
Can you change the control flow so that this error is caught elsewhere, and we only have the code_contains_strings call at the bottom of the function?
| def _uses_pyplot_show(code: str) -> bool: | ||
| """Check if code calls plt.show() or similar show() methods. | ||
|
|
||
| Uses AST analysis to robustly detect show() calls regardless of import | ||
| aliases (e.g., `import matplotlib.pyplot as mpl`). AST approach detects | ||
| actual method calls, avoiding false positives from string literals. | ||
| Falls back to string matching only if code doesn't parse. | ||
| """ | ||
| if _find_attribute_calls(code, ["show"]): | ||
| return True | ||
| try: | ||
| ast.parse(code) | ||
| except (SyntaxError, ValueError): | ||
| return _code_contains_strings(code, ["plt.show", ".show()"]) | ||
| return False |
There was a problem hiding this comment.
Are we okay with this having false-positives? Doesn't this check for any invocation of .show even if it's not plt?
| def _uses_pyplot_plot(code: str) -> bool: | ||
| """Check if code calls pyplot plotting functions. | ||
|
|
||
| Uses AST analysis to detect plot-related method calls. Handles import | ||
| aliases and detects actual method calls, avoiding false positives from | ||
| string literals or method references. Falls back to string matching | ||
| only if code doesn't parse. | ||
| """ | ||
| plot_methods = {"plot", "bar", "scatter", "hist", "imshow", "figure", "subplot"} | ||
| if _find_attribute_calls(code, list(plot_methods)): | ||
| return True | ||
| try: | ||
| ast.parse(code) | ||
| except (SyntaxError, ValueError): | ||
| return _code_contains_strings( | ||
| code, [f".{m}(" for m in plot_methods] + [f"plt.{m}" for m in plot_methods] | ||
| ) | ||
| return False |
There was a problem hiding this comment.
Same as above, there's a risk that false-positives are detected. Can this be combined with one of your other checks that checks for matplotlib being imported as well?
| def _calls_savefig(code: str) -> bool: | ||
| """Check if code calls plt.savefig() or fig.savefig(). | ||
|
|
||
| Uses AST analysis to robustly detect savefig() calls regardless of | ||
| how matplotlib was imported. Detects actual method calls, avoiding | ||
| false positives from string literals. Falls back to string matching | ||
| only if code doesn't parse. | ||
| """ | ||
| if _find_attribute_calls(code, ["savefig"]): | ||
| return True | ||
| try: | ||
| ast.parse(code) | ||
| except (SyntaxError, ValueError): | ||
| return _code_contains_strings(code, ["savefig"]) | ||
| return False |
| def get_unauthorized_imports( | ||
| code: str, allowed_imports: list[str] | None = None | ||
| ) -> list[str]: | ||
| """Extract unauthorized top-level imports from Python code.""" | ||
| if allowed_imports is None: | ||
| return [] | ||
|
|
||
| unauthorized: set[str] = set() | ||
| try: | ||
| tree = ast.parse(code) | ||
| except (SyntaxError, ValueError): | ||
| # Syntax errors are validated separately by dedicated validators. | ||
| return [] | ||
|
|
||
| for node in ast.walk(tree): | ||
| if isinstance(node, ast.Import): | ||
| for alias in node.names: | ||
| module = alias.name.split(".")[0] | ||
| if module not in allowed_imports: | ||
| unauthorized.add(module) | ||
| elif isinstance(node, ast.ImportFrom) and node.module: | ||
| module = node.module.split(".")[0] | ||
| if module not in allowed_imports: | ||
| unauthorized.add(module) | ||
|
|
||
| return sorted(unauthorized) |
There was a problem hiding this comment.
I believe I mentioned this previously, but is there a reason not to utilize our pre-existing allowed imports checker function?
There was a problem hiding this comment.
Yes, I can remove this file and use the get_unauthorized_imports in interpreter.py by making it public.
| return score | ||
|
|
||
|
|
||
| def extract_python_code(ctx: Context) -> ValidationResult: |
There was a problem hiding this comment.
This looks very similar to the function below it. Can you please describe why it's needed / why the two functions can't be combined?
There was a problem hiding this comment.
Why Both Are Needed:
- extract_python_code - For validators that check code properties (syntax, imports, plot requirements) which can be done on either source:
- If user called the python tool directly → use that code
- If user wrote markdown → use that code
- These validators run before or after tool invocation - _has_python_code_listing - For validators that actually execute code, which should only work with markdown:
- Execution validators must extract from markdown (the actual code text)
- Tool calls are metadata/structured, not plain text to execute
- This validator runs in the execution path (_python_executes_without_error)
Refactor to eliminate duplication
| def _code_parses(code: str) -> tuple[bool, str | None]: | ||
| """Check if code parses as valid Python. | ||
|
|
||
| Returns: | ||
| (True, None) if code parses | ||
| (False, error_message) if syntax error | ||
| """ | ||
| try: | ||
| ast.parse(code) | ||
| return True, None | ||
| except SyntaxError as e: | ||
| error_msg = f"Syntax error at line {e.lineno}: {e.msg}" | ||
| if e.text: | ||
| error_msg += f"\n {e.text.rstrip()}" | ||
| if e.offset: | ||
| error_msg += "\n " + " " * (e.offset - 1) + "^" | ||
| return False, error_msg |
There was a problem hiding this comment.
I think this is still unresolved.
| """Requirement factories for Python tool invocation and code validation. | ||
|
|
||
| This module provides generic requirements for Python-tool usage and code | ||
| correctness. Plotting-specific checks are exposed separately through | ||
| ``plotting.python_plotting_requirements(...)`` so they are not implied to be | ||
| universal Python-tool requirements. | ||
| """ |
There was a problem hiding this comment.
Please modify this docstring. I don't think this encompasses the plotting tools anymore.
| from ..tools.interpreter import StaticAnalysisEnvironment | ||
| from .imports import get_unauthorized_imports | ||
| from .plotting import python_plotting_requirements | ||
| from .python_reqs import extract_python_code |
There was a problem hiding this comment.
I think it would be helpful to add more context to this extract_python_code function (especially in the functions that call it). Do we even have a python tool that is invoked by that name? Is the python tool invoked / computed at the point in time that requirements are being checked?
If we keep this function as is, please add notes to the requirements that utilize it that explain how it looks for the python code.
There was a problem hiding this comment.
"extract_python_code" extracts python code in the context. The requirements verify if the python code meets their requirements.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
I believe all comment addressed. Thanks! |
AngeloDanducci
left a comment
There was a problem hiding this comment.
LGTM I'll let Jake give the final thumbs up given the changes there were more in depth.
markstur
left a comment
There was a problem hiding this comment.
suggested change in example
| if _find_attribute_calls(code, list(PYPLOT_PLOT_METHODS)): | ||
| return True | ||
| try: | ||
| ast.parse(code) |
There was a problem hiding this comment.
I think this is parsing code that has already been parsed, but I didn't look around much to see if it was worth reuse
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
|
||
| Prerequisites: | ||
| matplotlib must be installed for code execution to succeed: | ||
| $ uv pip install matplotlib |
There was a problem hiding this comment.
I just run it like this:
uv run --with matplotlib python docs/examples/tools/python_plotting_repair.py
And I think it is really cool that temporary deps work like that but I'm not sure how many people have bought into using --with
markstur
left a comment
There was a problem hiding this comment.
I think my feedback was resolved.
I added a new inline about running with --with which I happen to like but not sure how accepted it is (try it :) )
I don't know if Jake's feedback was covered. I didn't track it enough to know.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
|
@jakelorocco I tried to address all you comments. I might miss some but please review, again. Thanks! |
Requirement PR
Use this template when adding or modifying requirements in
mellea/stdlib/requirements/.Description
Add requirements for Python code generation.
Implementation Checklist
Base Class
Requirement- standard requirementALoraRequirement- uses specialized Intrinsic/Adapter for generation-based validationValidation Logic
validation_fndefined (if using Python-based validation)mellea/stdlib/tools/validatereturns aValidationResultwiththunkandcontextif using a backend to generatereasonandscorewhen possibleIntegration
mellea/stdlib/requirements/__init__.pyor, if you are adding a library of requirements, from your sub-moduleTesting
tests/requirements/Attribution