FIX: Does not check image provider credentials while conducting tasks that does not require image gen models#202
FIX: Does not check image provider credentials while conducting tasks that does not require image gen models#202xraywu wants to merge 4 commits into
Conversation
dippatel1994
left a comment
There was a problem hiding this comment.
Adds a clean 'none' dummy image provider for tasks that do not need imagegen. CLI overrides and README updates. Thanks @xraywu!
|
Hey @xraywu, CI is failing after the branch update:
|
dippatel1994
left a comment
There was a problem hiding this comment.
Thanks @xraywu — the underlying fix here is right. The paperbanana plot workflow doesn't need an image generation provider (matplotlib renders the plot), so requiring OPENAI_API_KEY / equivalent just to construct the pipeline is a real onboarding papercut. Introducing a "none" provider that resolves to DummyImageGen is a clean way to model "image gen not used in this workflow" without scattering conditional checks across the pipeline. This closes #201.
Before merge, a few things need attention.
Required changes
1. README diff is almost entirely auto-format noise, please revert
The README touches in this PR (--- → *** horizontal rules, padding spaces inside every table, replacing empty cells with <br />) look like a markdown formatter running with non-default settings. These changes:
- Break the visual style we already established across the README (we use
---for HR throughout the project). - Renumber the "How It Works" pipeline steps (Phase 0 reset from
0.to1., Phase 2 reset from4./5./6.to1./2./3.), which makes the prose reference "Steps 4-5 repeat" point at nothing. - Add ~150 lines of churn that have to be re-resolved on every future merge.
Please revert all README changes except the one that genuinely matters: documenting the new plot flags (--vlm-model, --max-iterations, --config) in the plot CLI section. Keep that as a focused, small diff.
If your editor is auto-formatting on save, consider disabling the markdown formatter for this repo, or scoping the formatter to the lines you intentionally touched.
2. Add at least one test for the new behaviour
The fix is one new file + a registry branch + a CLI rewrite, but there are no tests. Please add:
- A test that
ProviderRegistry.create_image_gen(Settings(image_provider="none"))returns aDummyImageGeninstance. - A test that calling
DummyImageGen.generate(...)raisesRuntimeError(regression guard — if the plot path ever accidentally calls it, we want to fail loudly, not silently produce a corrupt image). - A CLI-level smoke test that
paperbanana plot --data <csv> --intent <str>constructs settings successfully without an image API key set. The existingtests/test_features.pyandtests/test_providers/test_registry.pyare the right homes.
3. Missing newline at end of paperbanana/providers/image_gen/dummy.py
Ruff will catch this once CI runs (W292 no newline at end of file). Easy fix.
Confirmations
4. Refactor scope
You also refactored the plot command's settings construction from kwargs to a dict-based overrides pattern, and added --vlm-model, --max-iterations, --config flags. These are improvements, but they're a separate concern from the bug fix in #201. I'm OK keeping them in this PR since they're co-located, but please call them out in the PR description so reviewers and the changelog accurately reflect what's landing.
5. image_provider = "none" is set unconditionally for plot
That's correct behaviour for today (no plot path uses image gen), but if we ever add a plot mode that does call an image model (e.g. for chart annotation overlays), this hardcoded override will silently break it. Could you add a one-line comment in cli.py at the line where overrides["image_provider"] = "none" is set, explaining why it's hardcoded? Future-you (or me) will thank you.
Approval
Approving once the README is reverted to scoped changes only, the tests above are added, and the EOF newline is fixed.
Fixed #201 by adding a dummy image provider "none", so that it does not break the compatiblility of current workflow. Also added support for config files and more command line parameters that overrides settings and now plot command will be much more easy to use just as generate command.