Skip to content

FIX: Does not check image provider credentials while conducting tasks that does not require image gen models#202

Open
xraywu wants to merge 4 commits into
llmsresearch:mainfrom
xraywu:plot_cli_params
Open

FIX: Does not check image provider credentials while conducting tasks that does not require image gen models#202
xraywu wants to merge 4 commits into
llmsresearch:mainfrom
xraywu:plot_cli_params

Conversation

@xraywu
Copy link
Copy Markdown

@xraywu xraywu commented May 8, 2026

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.

Copy link
Copy Markdown
Member

@dippatel1994 dippatel1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adds a clean 'none' dummy image provider for tasks that do not need imagegen. CLI overrides and README updates. Thanks @xraywu!

@dippatel1994
Copy link
Copy Markdown
Member

Hey @xraywu, CI is failing after the branch update:

  • Lint has 2 errors (run ruff check && ruff format locally)
  • Tests failing including test_plot_settings_includes_generate_caption and others. Please pull main and fix the regressions.

Copy link
Copy Markdown
Member

@dippatel1994 dippatel1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. to 1., Phase 2 reset from 4./5./6. to 1./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 a DummyImageGen instance.
  • A test that calling DummyImageGen.generate(...) raises RuntimeError (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 existing tests/test_features.py and tests/test_providers/test_registry.py are 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Plot command requires image gen provider credentials even not used

2 participants