docs: add python / matplotlib requirement example#1293
Conversation
psschwei
left a comment
There was a problem hiding this comment.
a few nits but otherwise LGTM
| import mellea | ||
| from mellea.stdlib.components import Message | ||
| from mellea.stdlib.context import ChatContext | ||
| from mellea.stdlib.requirements.plotting.matplotlib import ( |
There was a problem hiding this comment.
nit: In this readme, this is from mellea.stdlib.requirements.plotting import PlotFileSaved, probably should use the same both places (not sure which is right)
There was a problem hiding this comment.
This one was wrong. Fixed it.
| print("=" * 70) | ||
|
|
||
| # Use provided options or defaults | ||
| csv_path = args.csv if args.csv else "/tmp/sample_data.csv" |
There was a problem hiding this comment.
would it make sense to use tempfile instead of hardcoding a path?
There was a problem hiding this comment.
Yes, it make sense. Updated it.
| "Nancy", | ||
| "Oscar", | ||
| "Piper", | ||
| "Quinn", |
There was a problem hiding this comment.
also on L179, is it ok to duplicate or should we use another Q name here?
| print(f"\n ✓ Graph saved to: {graph_path}") | ||
| print(f" File size: {graph_path.stat().st_size} bytes") | ||
| else: | ||
| print(f"\n ⚠ Graph file not found at {graph_path}") |
There was a problem hiding this comment.
The conftest passes the test when the script exits 0, and process_user_request always exits 0 — there's no sys.exit or raise when the graph isn't produced, just a print + return. So even with matplotlib installed, if the pipeline fails for any reason the test is recorded as passing whilst producing nothing. Worth raising here so failures are visible:
| print(f"\n ⚠ Graph file not found at {graph_path}") | |
| if not graph_path.exists(): | |
| raise RuntimeError(f"Graph was not created at {graph_path}") | |
| print(f"\n ✓ Graph saved to: {graph_path}") | |
| print(f" File size: {graph_path.stat().st_size} bytes") |
| PythonCodeExtraction(), | ||
| PythonSyntaxValid(), | ||
| PythonExecutionReq( | ||
| execution_tier="local", |
There was a problem hiding this comment.
execution_tier="local" triggers a library-level warning about running untrusted code without container isolation, and the README lists "CI/CD environments" as a use case for this example. Might be worth a comment pointing readers to execution_tier="docker" for untrusted inputs — keeps the local demo useful whilst teaching the safer pattern. (Context: commit 85c447e recently tightened the project's default posture here.)
There was a problem hiding this comment.
I'll address this as a follow up.
There was a problem hiding this comment.
Can we add a link to the follow-up issue here for traceability?
|
Tiny one: the PR title has a typo — "requrement" should be "requirement". Worth fixing before merge as it ends up in the commit message. |
| import mellea | ||
| from mellea.stdlib.components import Message | ||
| from mellea.stdlib.context import ChatContext | ||
| from mellea.stdlib.requirements.plotting import MatplotlibHeadlessBackend, PlotFileSaved |
There was a problem hiding this comment.
matplotlib isn't a project dependency, so without it the repair loop spins 5 times against a ModuleNotFoundError (no specific handler for this case), then exits with ⚠ Graph file not found and no hint of the cause. The project convention (AGENTS.md §5) is to wrap optional imports in a try/except ImportError with a friendly message — that would both tell the user what to install and let the CI conftest skip the test cleanly (it already converts ImportError in subprocess stderr to pytest.skip). The README also needs an install step for the new example — the existing Requirements section refers to the AST-only examples and doesn't cover this one.
There was a problem hiding this comment.
Changes made:
- Friendly dependency error: Added try/except ImportError with a clear message telling users exactly what to install
- README prerequisites: Added a "Prerequisites" section that documents:
- How to start Ollama with the required model
- How to install matplotlib and numpy
- Clear requirements distinction: Updated the final "Requirements" section to explicitly separate the needs of each example
planetf1
left a comment
There was a problem hiding this comment.
Thanks for the example — the pipeline approach is nicely structured and the README additions are clear.
Two things I'd want addressed before merge:
- matplotlib guard —
matplotlibisn't a project dependency so the example can't succeed on a clean checkout. The repair loop exhausts silently with no useful message to the user (see inline comment). - Silent test pass — the conftest passes the test on exit 0, and
process_user_requestalways exits 0 even when no graph is produced. The e2e test gives a false green in that case (see inline comment).
The other inline comments (execution tier caveat, README requirements section) are informational — happy for those to be addressed or tracked as follow-ups.
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>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
| # NOTE: Code execution has already occurred as a side effect of requirement validation. | ||
| # When m.instruct() is called with PythonExecutionReq, the validation_fn executes the | ||
| # code in a subprocess to verify it runs without errors. The code execution below is | ||
| # documented for educational purposes to show what happens during validation, but is | ||
| # not needed since validation already ran the code. To verify the graph was created, | ||
| # we only need to check if the output file exists (see process_user_request line ~310). | ||
| # | ||
| # def execute_python_code(code: str, timeout: int = 10) -> dict: | ||
| # """Execute Python code in a subprocess and capture output. | ||
| # | ||
| # Args: | ||
| # code: Python code to execute | ||
| # timeout: Maximum execution time in seconds | ||
| # | ||
| # Returns: | ||
| # dict with 'success', 'output', and 'error' keys | ||
| # """ | ||
| # try: | ||
| # with tempfile.NamedTemporaryFile(mode="w", suffix=".py", delete=False) as f: | ||
| # f.write(code) | ||
| # temp_file = f.name | ||
| # | ||
| # try: | ||
| # result = subprocess.run( | ||
| # [sys.executable, temp_file], | ||
| # capture_output=True, | ||
| # text=True, | ||
| # timeout=timeout, | ||
| # ) | ||
| # | ||
| # return { | ||
| # "success": result.returncode == 0, | ||
| # "output": result.stdout, | ||
| # "error": result.stderr, | ||
| # "return_code": result.returncode, | ||
| # } | ||
| # finally: | ||
| # Path(temp_file).unlink() | ||
| # | ||
| # except subprocess.TimeoutExpired: | ||
| # return { | ||
| # "success": False, | ||
| # "output": "", | ||
| # "error": f"Code execution timed out after {timeout} seconds", | ||
| # "return_code": -1, | ||
| # } | ||
| # except Exception as e: | ||
| # return {"success": False, "output": "", "error": str(e), "return_code": -1} |
There was a problem hiding this comment.
would it make sense to put this in the readme for this example and then link to it there if folks want to know what happened?
| # NOTE: Code execution already occurred as a side effect of PythonExecutionReq | ||
| # validation during m.instruct(). Because PythonExecutionReq uses execution_tier="local" | ||
| # (not "static"), the code was executed in a subprocess during validation. The lines | ||
| # below are kept as comments to show what would happen if we were executing separately: | ||
| # | ||
| # print("\nExecuting code...") | ||
| # result = execute_python_code(code, timeout=15) | ||
| # | ||
| # if not result["success"]: | ||
| # print(f" ✗ Error during execution: {result['error']}") | ||
| # return | ||
| # | ||
| # print(" ✓ Code executed successfully") | ||
| # if result["output"]: | ||
| # print(f"\n Output: {result['output']}") |
There was a problem hiding this comment.
same comment here, re: linking out to readme
Signed-off-by: Akihiko Kuroda <akihikokuroda2020@gmail.com>
Pull Request
Issue
Fix: #1025
Description
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.