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.
📝 WalkthroughWalkthroughTOPP input handling now accepts and persists boolean flag parameter names. Command construction loads these definitions and emits enabled flags without values, while omitting disabled or empty parameters and preserving list and multiline expansion. ChangesTOPP Flag Parameters
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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
| flag_map = self.parameter_manager.get_parameters_from_json().get("_flag_params", {}) | ||
| if not flag_map: | ||
| flag_map = st.session_state.get("_topp_flag_params", {}) | ||
| flag_params: set = set(flag_map.get(params_key, [])) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the per-tool fallback logic for flag parameters.
The current logic falls back to st.session_state only if _flag_params is completely missing or empty in params.json. If another tool has saved its flags, flag_map evaluates to True, and the fallback won't be evaluated for the current tool (params_key), potentially ignoring its session state flags.
Evaluate the fallback at the params_key level instead.
💻 Proposed fix
- flag_map = self.parameter_manager.get_parameters_from_json().get("_flag_params", {})
- if not flag_map:
- flag_map = st.session_state.get("_topp_flag_params", {})
- flag_params: set = set(flag_map.get(params_key, []))
+ flag_map = self.parameter_manager.get_parameters_from_json().get("_flag_params", {})
+
+ flag_list = flag_map.get(params_key)
+ if flag_list is None:
+ flag_list = st.session_state.get("_topp_flag_params", {}).get(params_key, [])
+
+ flag_params: set = set(flag_list)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| flag_map = self.parameter_manager.get_parameters_from_json().get("_flag_params", {}) | |
| if not flag_map: | |
| flag_map = st.session_state.get("_topp_flag_params", {}) | |
| flag_params: set = set(flag_map.get(params_key, [])) | |
| flag_map = self.parameter_manager.get_parameters_from_json().get("_flag_params", {}) | |
| flag_list = flag_map.get(params_key) | |
| if flag_list is None: | |
| flag_list = st.session_state.get("_topp_flag_params", {}).get(params_key, []) | |
| flag_params: set = set(flag_list) |
🤖 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/CommandExecutor.py` around lines 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.
| # Regular parameter: skip empty/None, append value otherwise | ||
| if v == "" or v is None: | ||
| continue | ||
| command += [f"-{k}"] | ||
| if isinstance(v, str) and "\n" in v: | ||
| command += v.split("\n") | ||
| else: | ||
| command += [str(v)] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix list handling and omit empty lists in command construction. Both loops formatting non-flag parameters (merged_params and custom_params) incorrectly process list values. merged_params converts list objects into string representations (e.g., "['a', 'b']") instead of expanding them as distinct CLI arguments. Additionally, both loops fail to skip empty lists ([]), which causes them to append a flag (e.g. -k) without providing a corresponding value, leading to TOPP parsing errors.
src/workflow/CommandExecutor.py#L308-L315: Update the skip condition to omit empty lists (v == "" or v is None or (isinstance(v, list) and not v)) and add anelif isinstance(v, list): command += [str(x) for x in v]block to properly expand list values.src/workflow/CommandExecutor.py#L326-L332: Update the skip condition to identically omit empty lists (if v == "" or v is None or (isinstance(v, list) and not v):).
📍 Affects 1 file
src/workflow/CommandExecutor.py#L308-L315(this comment)src/workflow/CommandExecutor.py#L326-L332
🤖 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/CommandExecutor.py` around lines 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.
1 participant
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