Repository navigation
Conversation
…talog - Add navigation README for docs/examples/sampling/ - Standardize header docstrings in sampling and agent scripts - Index react_using_mellea.py in docs/examples/agents/README.md - Register sampling/, requirements/, and audio_text_models/ in docs/docs/examples/index.md Assisted-by: IBM Bob Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
planetf1
left a comment
There was a problem hiding this comment.
Useful docs-only change: it makes sampling/ and the stdlib react() example discoverable, and the READMEs and index rows match the source. I ran both sampling examples against Ollama and they run. Three non-blocking points below, plus one follow-up suggestion.
Separately, not for this PR: presets.py:220 tells users to run uv sync --extra plotting, but no such extra exists in pyproject.toml.
| | File | Description | | ||
| | :--- | :--- | | ||
| | [`python_code_generation_preset.py`](./python_code_generation_preset.py) | Python code generation preset bundling extraction, syntax, and execution requirements with automatic repair feedback. | | ||
| | [`python_plotting_preset.py`](./python_plotting_preset.py) | Matplotlib plotting preset bundling headless backend setup, plot file output validation, and sandbox execution. | |
There was a problem hiding this comment.
These examples need a running Ollama with the default model (granite4.2:3b), and the plotting ones need matplotlib, which isn't in any extra. example_without_output_path is the only one that runs generated code in Docker, so it also needs llm-sandbox (mellea[sandbox]), a running Docker daemon, and a sandbox image that runs on your machine. The default image is amd64-only, which can matter on Apple Silicon. A short prerequisites line would help.
It would also help that, when the sandbox can't run, the failure is hard to spot. The run reports Valid: 5/6 with the unmet requirement shown only as "sandbox execution with policy", and the underlying error never appears in the output. The script still prints "✓ completed", because it doesn't check the sampling result.
There was a problem hiding this comment.
Updated the scripts to include ollama and dependency install commands in their Prerequisites. I think this helps towards keeping the examples self-contained and in case future examples are added under this directory that may not need the same prerequisites.
There was a problem hiding this comment.
Partially fixed. I walked through these as someone running the examples fresh:
ollama pullis listed beforeollama serve, but pull needs the server up. Worth phrasing for users running Ollama as a service or via the desktop app too, e.g. "Ollama running (service, desktop app, orollama serve), thenollama pull granite4.2:3b".pip install matplotlib numpybeside auv runUsage line is a trap: uv-managed venvs don't includepip, and a systempipinstalls into a different environment thanuv runuses.uv pip install matplotlib numpyis the drop-in fix. (Aplottingextra would also fix the--extra plottingreference inpresets.py:220, but that's a follow-up.)- The plotting module's Prerequisites are incomplete for its own Usage line: running the module executes
main(), which includesexample_without_output_path, so it needs Docker anduv sync --extra sandbox. Those are only in that function's docstring. The amd64-only default sandbox image note is also still missing; on Apple Silicon that example can't pass as shipped even with Docker running, and the preset installs no packages into the container, so matplotlib may be missing there too.
On failure visibility: the docstring note is good, but the behaviour is unchanged. A newcomer sees four check lines, a Valid: 5/6 buried in the output, then a completed message, and reasonably concludes everything passed. SamplingResult.success exists (core/sampling.py:41), so checking it before printing a pass would close the gap. If documenting rather than fixing is preferred, I'd drop "fails silently" in the note, since the next sentence describes exactly what is reported.
P.S. while checking model tags: requirements/README.md:31 says ollama pull granite4.1:3b, but its examples use the default session, which is granite4.2:3b (session.py:271). Anyone following that README pulls the wrong 2 GB model and then triggers an auto-pull of the right one.
There was a problem hiding this comment.
I reordered to ollama serve/ollama pull across all five scripts and switched to uv pip install matplotlib numpy.
The plotting module's Prerequisites now include uv sync --extra sandbox, and the docstring notes the Docker requirement and the amd64-only image.
On failure visibility, I went with the second option for this PR and dropped the "fails silently" message. The --extra plotting hint is tracked in the issue I opened #1763.
There was a problem hiding this comment.
Thanks — the Prerequisites blocks and the "fails silently" rewording read well. Three residuals from this thread:
-
requirements/README.mdstill points at the wrong model:granite4.1:3bat lines 31, 34, 144, 147, and 417. Its examples use the default session, which isgranite4.2:3b(session.py:271,model_ids.py:119). Anyone following that README pulls the wrong ~2 GB model, then the session auto-pulls the right one. Four-line fix, and it's a README this PR already touches. -
Whether
example_without_output_pathcan pass as shipped is still open, and I suspect it can't even with Docker running.DOCKER_POLICY(execution_policy.py:214) enables package installation but sets nopackageslist (packagesdefaults to empty,execution_policy.py:113), soLLMSandboxEnvironmentinstalls nothing into the container (interpreter.py:354). If llm-sandbox's default image (ghcr.io/vndee/sandbox-python-311-bullseye) doesn't ship matplotlib, the sandbox requirement fails for a reason no prerequisite fixes. Could you verify, or note it in the docstring? I haven't pulled the image to check. -
Minor: the reorder to
ollama serve/ollama pullfixes the order but isn't runnable as a block, sinceserveblocks the terminal. Phrasing like "Ollama running (service, desktop app, orollama serve), thenollama pull granite4.2:3b" would cover it. Happy to drop this if you prefer the current form.
Separately, while checking the head merge: sampling/ is now listed twice in docs/docs/examples/index.md — the pre-existing row at line 83 and the row this PR adds at line 87. The tip commit deduped the requirements/ row; this one looks missed.
There was a problem hiding this comment.
Addressed 1 and 2 along with the sampling/ duplicate. I'll leave ollama commands as-is for now, it's at least implied that an ollama server should be running by the command itself.
…ings Assisted-by: IBM Bob Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Assisted-by: IBM Bob Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com>
…ndbox image Signed-off-by: Rafael Vasquez <rafvasq21@gmail.com> Assisted-by: IBM Bob
Pull Request
Issue
Towards #1746
Docstrings
Prerequisites:andUsage:blocks.READMEs
sampling/README.md: defines a preset, links torequirements/, and has an## Examplestable.agents/README.md: a short description and an## Examplestable.Other fixes
react_using_mellea.py: documented thelangchain-communityandddgsdependencies.python_plotting_preset.pythat described sandboxed execution while passinguse_sandbox=False.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.