feat: python tools requirement#1040
feat: python tools requirement#1040akihikokuroda wants to merge 3 commits intogenerative-computing:mainfrom
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.
| return None | ||
|
|
||
|
|
||
| def _get_unauthorized_imports( |
There was a problem hiding this comment.
There was a problem hiding this comment.
Yes, it is. A new helper function is added that is used by both.
| return validate | ||
|
|
||
|
|
||
| def _make_output_limit_validator( |
There was a problem hiding this comment.
Does this have an associated test?
Bit hazy on the stdout and stderr here - in this case the ctx.last_output() is a ModelOutputThunk I think? Does that have stdout/err?
There was a problem hiding this comment.
Here is what the claude found about the stdout and stderr:
Your intuition is correct. Here's the precise behavior:
- ctx.last_output() returns ModelOutputThunk | None
- ModelOutputThunk doesn't define stdout/stderr in its class
- These attributes are dynamically injected at runtime when tools execute and return ExecutionResult
- The hasattr() checks (lines 366-369) are defensive and correct — they gracefully handle both cases (tool execution with output vs. pure LLM response)
There was a problem hiding this comment.
No, there was no tests. New tests are added now.
| print("=" * 70) | ||
| print("Testing Granite 4.1's ability to repair plotting failures") | ||
| print("=" * 70) | ||
| print(f"Task: Create a plot of sin(x) and save to {output_path}\n") |
There was a problem hiding this comment.
suggest using a task variable to share the task string used in actual description with here.
|
|
||
| async def main(): | ||
| """Run the canonical plotting repair example.""" | ||
| import tempfile |
There was a problem hiding this comment.
move all the imports to the top
| module_name = node.module.split(".")[0] | ||
| if module_name not in allowed_imports: | ||
| unauthorized.append(module_name) | ||
| except (SyntaxError, ValueError): |
There was a problem hiding this comment.
why not raise these?
This pass means that the function did not do its job.
Most likely this code is unusable elsewhere anyway, but if that external assumption is true it just snuck past the unauthorized import check.
There was a problem hiding this comment.
Comments are added explaining this.
| headless_backends = ("Agg", "Svg", "Cairo", "PDF", "PS", "WebAgg", "nbAgg") | ||
| for backend in headless_backends: | ||
| if ( | ||
| f"matplotlib.use('{backend}')" in code |
There was a problem hiding this comment.
commented out code would be a false positive.
You might consider using python tokenize to strip comments.
Probably strip docstring too.
There was a problem hiding this comment.
Added simpler way to strip the comments.
| m = mellea.start_session() | ||
|
|
||
| # Create requirements bundle for plotting validation | ||
| # Allows matplotlib import (no output_path = skip file creation check) |
There was a problem hiding this comment.
This example is too confusing. It always fail w/o output_path because there is a bunch of code to ensure that the output file exists. Then I add output_path and still consistently fail to get a file.
This might be intentional (fail to write file), but if so it needs to be clearer because right now it looks like a bad example that needs fixing/debugging.
There was a problem hiding this comment.
Yes, this is intentional. I added comment explaining a little better.
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
3 participants
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