feat: python tools requirement#1040
feat: python tools requirement#1040akihikokuroda wants to merge 1 commit 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.
| 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?
| 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.
| 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.
| 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.
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