Add flag parameter support for TOPP tools#397
Conversation
Allows input_TOPP() to designate parameters as flags (present/absent) instead of key-value pairs, persisted to params.json and session_state so run_topp() can correctly build the command line.
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/workflow/StreamlitUI.py (1)
827-833: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid mutable data structures for argument defaults.
Using mutable defaults like
[]or{}can lead to unexpected behavior since the same instance is shared across all function calls. While they aren't mutated in this specific method, it's best practice to default toNoneand initialize the mutable structures within the function body.♻️ Proposed refactor
- exclude_parameters: List[str] = [], - include_parameters: List[str] = [], - flag_parameters: List[str] = [], + exclude_parameters: List[str] = None, + include_parameters: List[str] = None, + flag_parameters: List[str] = None, display_tool_name: bool = True, display_subsections: bool = True, display_subsection_tabs: bool = False, - custom_defaults: dict = {}, + custom_defaults: dict = None,(Note: Ensure you also initialize these to empty lists/dicts inside the function if they are
None)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/workflow/StreamlitUI.py` around lines 827 - 833, Update the affected function’s exclude_parameters, include_parameters, flag_parameters, and custom_defaults arguments to default to None instead of mutable list or dictionary instances, then initialize each to an empty list or dictionary inside the function when it is None. Preserve the existing behavior for callers that provide values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/workflow/CommandExecutor.py`:
- Around line 308-315: The non-flag parameter loops in CommandExecutor must omit
empty lists and expand non-empty lists into separate CLI arguments. In
src/workflow/CommandExecutor.py lines 308-315, update the skip condition and add
list handling before scalar conversion; apply the same empty-list skip condition
in lines 326-332, while preserving existing string newline handling.
- Around line 274-278: Update the flag parameter selection in the relevant
CommandExecutor method so the fallback from st.session_state is evaluated when
flag_map has no entry for the current params_key, not only when the entire flag
map is empty. Preserve any existing params.json flags for that tool and
construct flag_params from the selected per-tool values.
---
Nitpick comments:
In `@src/workflow/StreamlitUI.py`:
- Around line 827-833: Update the affected function’s exclude_parameters,
include_parameters, flag_parameters, and custom_defaults arguments to default to
None instead of mutable list or dictionary instances, then initialize each to an
empty list or dictionary inside the function when it is None. Preserve the
existing behavior for callers that provide values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e933d71a-162b-4d6e-8e2d-e337f259b1f6
📒 Files selected for processing (2)
src/workflow/CommandExecutor.pysrc/workflow/StreamlitUI.py
Exercise run_topp()'s flag-aware command construction and the flag-source contract written by input_TOPP(). Working-behaviour tests cover: bare flag emission for truthy bool/"true" values, omission for falsy/"false", empty and None regular params skipped, 0/0.0 preserved, multiline splitting, custom_params scalar/list/flag handling, and params.json-vs-session_state precedence. A TestKnownCodeRabbitBugs group asserts the correct behaviour for the two CodeRabbit findings (per-tool flag fallback; list expansion / empty-list skipping). Those tests fail against the current code, documenting the defects until they are fixed.
The continuous-integration workflow (which runs `pytest test_gui.py tests/`) was triggered on push only, so pull requests from forks never executed the tests/ suite as a check. Add the pull_request trigger so the test job gates PRs as well.
Address both CodeRabbit findings on run_topp() (CommandExecutor.py): - Per-tool flag fallback: consult session_state per params_key instead of only when the whole _flag_params map is empty, so one tool's saved flags no longer shadow another tool's session flags. - List handling in the non-flag branches: skip empty lists in both the merged and custom loops, and expand non-empty merged lists into separate args instead of stringifying them as "['a', 'b']". Move the four tests that pinned these findings out of TestKnownCodeRabbitBugs into the behaviour classes they now guard (they pass with the fixes applied). Also fix a pre-existing, unrelated CI failure in test_queue_manager_cancel.py: test_stopped_status_is_mapped_in_get_job_info compared JobStatus enum members by identity via a re-import, which mismatches once other test modules pop src.workflow.* from sys.modules. Compare .name instead — reload-proof and semantically equivalent.
|
Added some tests and fixed the issues flagged by coderabbit. Thanks for your contribution. |
2 participants
Summary
flag_parametersargument toinput_TOPP()so specific parameters can be designated as CLI flags (e.g.-force,-no_progress) that are passed by presence only, without a valueparams.json(_flag_params) andsession_state["_topp_flag_params"], so it can be restored fromparams.jsoneven after a session restartChanges
src/workflow/StreamlitUI.pyflag_parameters: List[str] = []parameter toinput_TOPP()_topp_flag_params(session_state) and_flag_params(params.json)src/workflow/CommandExecutor.pyrun_topp(), loads the flag list fromparams.jsonfirst, falling back tosession_stateif not foundmerged_params/custom_paramscommand-building logic so flag parameters are added as-keyonly when truthy, while all other parameters keep the existing behaviorSummary by CodeRabbit