support flag parameters without valuesAllow flags to work with only key names, omitting values.#391
support flag parameters without valuesAllow flags to work with only key names, omitting values.#391hjn0415a wants to merge 1 commit into
Conversation
…only key names, omitting values.
📝 WalkthroughWalkthroughThe PR extends TOPP command generation to properly handle boolean parameters by extracting them from tool ChangesBoolean Parameter Support for TOPP Command Generation
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/workflow/CommandExecutor.py (1)
219-248:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
tool_instance_nameparameter is declared but never used.The parameter is documented as enabling lookup from
params.jsonusing instance-specific keys, but the method body always usestool(line 296:if tool in params.keys()). This means the feature for supporting multiple instances of the same TOPP tool won't work as intended.Likely fix: use
tool_instance_name or toolwhen looking up parameters.Additionally, static analysis flags two minor issues:
custom_params: dict = {}uses a mutable default (B006)tool_instance_name: str = Noneshould bestr | None(RUF013)Proposed fix
- def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}, tool_instance_name: str = None) -> bool: + def run_topp(self, tool: str, input_output: dict, custom_params: dict | None = None, tool_instance_name: str | None = None) -> bool:And at the start of the method body:
if custom_params is None: custom_params = {} param_key = tool_instance_name or toolThen use
param_keyinstead oftoolwhen accessingparams(e.g., line 296).🤖 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 219 - 248, The run_topp method currently ignores the tool_instance_name parameter and uses a mutable default for custom_params and a non-PEP604 union type; fix by normalizing inputs at the start of run_topp: treat custom_params safely (e.g., if custom_params is None then set to an empty dict) and compute a lookup key like param_key = tool_instance_name or tool, then use param_key wherever params are accessed (e.g., replace checks like if tool in params.keys() with if param_key in params.keys()); also update the function signature/type-hint to use a non-mutable default (None) for custom_params and use the union type for tool_instance_name (str | None).
🧹 Nitpick comments (2)
src/workflow/CommandExecutor.py (1)
298-307: 💤 Low valueInconsistent handling of boolean parameters with empty string values.
When a boolean parameter has
v == "":
- Line 299's condition
k in topp_bool_flag_param_keys and v != ""isFalse- It falls through to lines 308-315, which adds
-{k}without a valueThis means empty-string boolean params emit the flag (enabled behavior), while explicit
"false"params are omitted. This may be intentional, but it's inconsistent and could confuse users.Also, the comment on line 300 appears to have incorrect indentation relative to the code block it describes.
Suggested cleanup
if k in topp_bool_flag_param_keys and v != "": - # CLI flag: include "-k" only when enabled + # CLI flag: include "-k" only when enabled if isinstance(v, str): is_enabled = v.lower() == "true" else: is_enabled = bool(v) if is_enabled: command += [f"-{k}"] continue + elif k in topp_bool_flag_param_keys: + # Empty string boolean param: skip entirely (treat as disabled) + continue🤖 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 298 - 307, The boolean-flag handling in CommandExecutor.py around the topp_bool_flag_param_keys check treats v == "" inconsistently (empty string falls through and emits the flag while "false" omits it); update the logic that processes topp_bool_flag_param_keys (the branch that inspects k and v and computes is_enabled) to treat empty-string values as disabled (i.e., parse v=="" same as false) so flags are only added when is_enabled is True, and remove the fall-through that adds f"-{k}" for empty strings; also fix the misplaced/indented comment so it sits directly above the boolean-flag branch for clarity (referencing variables: topp_bool_flag_param_keys, k, v, is_enabled, and command).src/workflow/ParameterManager.py (1)
6-18: ⚡ Quick winConsider using a hardened XML parser for improved security posture.
Line 17 uses
xml.etree.ElementTreeto parse.inifiles. While these files are currently generated by internal TOPP tools (not untrusted user inputs), usingdefusedxmlwould be more robust against potential XXE vulnerabilities. However, note thatdefusedxmlis not currently a project dependency and would need to be added.-import xml.etree.ElementTree as ET +from defusedxml import ElementTree as ETThis would require adding
defusedxmlto the project dependencies.🤖 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/ParameterManager.py` around lines 6 - 18, The XML parsing in bool_param_paths_from_param_xml_ini currently uses xml.etree.ElementTree which can be vulnerable to XXE; switch to a hardened parser by adding defusedxml to project dependencies and updating the import/usage to use defusedxml's ElementTree (e.g. import ElementTree from defusedxml) and replace ET.parse(ini_path).getroot() in bool_param_paths_from_param_xml_ini with the defusedxml equivalent so parsing is safe; ensure dependency is added to your packaging (requirements/pyproject) and run tests to confirm no behavioral changes.
🤖 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/ParameterManager.py`:
- Around line 90-94: The code currently unions new bool params into the session
set (using bool_pairs_session_key()) causing stale params to persist when a
tool's .ini removes flags; instead, compute the current set of bool pairs for
the given tool by calling bool_param_paths_from_param_xml_ini(ini_path, tool),
remove any existing entries in st.session_state[sk] that belong to this tool
(matching the first element of the tuple), then add the current tuples (tool,
short) or simply replace st.session_state[sk] entries for that tool with the new
ones; update the logic around bool_pairs_session_key(),
bool_param_paths_from_param_xml_ini, ini_path and tool to ensure removal of old
tool-specific entries before adding the fresh list.
- Around line 80-83: The get_bool_param_pairs method currently calls
st.session_state.get(...) which throws RuntimeError outside of a Streamlit
session; guard this call the same way as _merge_bool_params_from_ini does (e.g.,
check for an active session or catch RuntimeError) and return the default empty
set when no session exists; update get_bool_param_pairs to first verify
Streamlit session availability before accessing st.session_state.get using the
same check/pattern used in _merge_bool_params_from_ini so behavior is
consistent.
---
Outside diff comments:
In `@src/workflow/CommandExecutor.py`:
- Around line 219-248: The run_topp method currently ignores the
tool_instance_name parameter and uses a mutable default for custom_params and a
non-PEP604 union type; fix by normalizing inputs at the start of run_topp: treat
custom_params safely (e.g., if custom_params is None then set to an empty dict)
and compute a lookup key like param_key = tool_instance_name or tool, then use
param_key wherever params are accessed (e.g., replace checks like if tool in
params.keys() with if param_key in params.keys()); also update the function
signature/type-hint to use a non-mutable default (None) for custom_params and
use the union type for tool_instance_name (str | None).
---
Nitpick comments:
In `@src/workflow/CommandExecutor.py`:
- Around line 298-307: The boolean-flag handling in CommandExecutor.py around
the topp_bool_flag_param_keys check treats v == "" inconsistently (empty string
falls through and emits the flag while "false" omits it); update the logic that
processes topp_bool_flag_param_keys (the branch that inspects k and v and
computes is_enabled) to treat empty-string values as disabled (i.e., parse v==""
same as false) so flags are only added when is_enabled is True, and remove the
fall-through that adds f"-{k}" for empty strings; also fix the
misplaced/indented comment so it sits directly above the boolean-flag branch for
clarity (referencing variables: topp_bool_flag_param_keys, k, v, is_enabled, and
command).
In `@src/workflow/ParameterManager.py`:
- Around line 6-18: The XML parsing in bool_param_paths_from_param_xml_ini
currently uses xml.etree.ElementTree which can be vulnerable to XXE; switch to a
hardened parser by adding defusedxml to project dependencies and updating the
import/usage to use defusedxml's ElementTree (e.g. import ElementTree from
defusedxml) and replace ET.parse(ini_path).getroot() in
bool_param_paths_from_param_xml_ini with the defusedxml equivalent so parsing is
safe; ensure dependency is added to your packaging (requirements/pyproject) and
run tests to confirm no behavioral changes.
🪄 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: b3cbb926-c96f-4f75-b3fc-bbd14747b380
📒 Files selected for processing (3)
.gitignoresrc/workflow/CommandExecutor.pysrc/workflow/ParameterManager.py
| def get_bool_param_pairs(self) -> set: | ||
| """Return the cached set of (tool, param path) bool params; empty set if none.""" | ||
| return st.session_state.get(self.bool_pairs_session_key(), set()) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/workflow/ParameterManager.py | head -100Repository: OpenMS/streamlit-template
Length of output: 4854
🏁 Script executed:
rg "get_bool_param_pairs" --type py -B 3 -A 3Repository: OpenMS/streamlit-template
Length of output: 665
🏁 Script executed:
rg "get_bool_param_pairs\(\)" --type pyRepository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "get_bool_param_pairs" 2>/dev/nullRepository: OpenMS/streamlit-template
Length of output: 104
🌐 Web query:
Streamlit st.session_state RuntimeError no active session
💡 Result:
Streamlit error “RuntimeError: no active session” (or similarly worded “Session state does not function when running a script without streamlit run”) happens when you access st.session_state outside an active Streamlit runtime session. Most common causes 1) Running the file as a normal Python script - st.session_state only works when the app is started by Streamlit (i.e., via streamlit run ...), because session state is tied to Streamlit’s WebSocket/runtime session. The Streamlit docs explain that session state is for “each user session” and is reset when the WebSocket/session is reset (e.g., refresh/navigation). [1] - If you run your script with python your_script.py, Streamlit emits a warning that session state does not function without streamlit run, followed by failures when you access attributes/keys. [2][3][4] 2) Code path executed during “startup”/external execution (e.g., debugger, background thread, import-time execution) - Community reports show that session state can be “reconstructed cleanly” or lost when code is executed outside the Streamlit-run context (e.g., debugging outside streamlit run). [5] 3) You’re in a session, but the specific key was never initialized yet - If you access a key that hasn’t been set in the current session, you’ll get a “no key … Did you forget to initialize it?” exception; this is separate from “no active session”, but it often appears during first run / first rerender. Initialize with a guard like if 'key' not in st.session_state: .... [1] How to fix A) Ensure you start the app correctly - Start with: - streamlit run path/to/app.py - Do not run with python app.py. This is exactly what the warning and error indicate. [2][4][3] B) Ensure session_state usage happens inside the Streamlit script execution - Avoid accessing/setting st.session_state at module import time if that code may run in a non-Streamlit context. - Avoid accessing session state from code that is executed outside Streamlit’s app lifecycle (e.g., separate background worker that is not managed by Streamlit’s runtime). Session state is tied to the Streamlit session/WebSocket. [1] C) Initialize keys before access - At the top of your app code (inside the Streamlit run), initialize keys before reading them, e.g.: - if 'foo' not in st.session_state: st.session_state['foo'] = default This is the documented initialization pattern. [1] Quick diagnosis checklist - Are you launching the app with streamlit run? If not, that’s the root cause. [2][3][4] - Do you see the warning about “Session state does not function when running a script without streamlit run”? That points directly to an inactive/no-runtime session. [2][6][3] - If you are definitely using streamlit run, check that the access is not happening in code that runs outside Streamlit’s lifecycle, and that you initialize keys with guards. [1][5]
Citations:
- 1: https://docs.streamlit.io/develop/api-reference/caching-and-state/st.session_state
- 2: https://stackoverflow.com/questions/78127387/keyerror-when-accessing-streamlit-session-state-in-python-script-despite-initial
- 3: streamlit.state.session_state: Session state does not function when running a script without
streamlit runstreamlit/streamlit#4543 - 4: https://stackoverflow.com/questions/76239464/how-to-properly-set-st-session-state-in-streamlit
- 5: Data placed in session_state is lost when debugging outside of Streamlit run streamlit/streamlit#5618
- 6: https://discuss.streamlit.io/t/st-session-not-initializing/53566
Guard get_bool_param_pairs() for non-Streamlit contexts.
The method calls st.session_state.get() which raises RuntimeError when no active Streamlit session exists. Since _merge_bool_params_from_ini() already guards against this case, apply the same pattern for consistency.
🔧 Suggested change
def get_bool_param_pairs(self) -> set:
"""Return the cached set of (tool, param path) bool params; empty set if none."""
- return st.session_state.get(self.bool_pairs_session_key(), set())
+ try:
+ return st.session_state.get(self.bool_pairs_session_key(), set())
+ except RuntimeError:
+ return set()📝 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.
| def get_bool_param_pairs(self) -> set: | |
| """Return the cached set of (tool, param path) bool params; empty set if none.""" | |
| return st.session_state.get(self.bool_pairs_session_key(), set()) | |
| def get_bool_param_pairs(self) -> set: | |
| """Return the cached set of (tool, param path) bool params; empty set if none.""" | |
| try: | |
| return st.session_state.get(self.bool_pairs_session_key(), set()) | |
| except RuntimeError: | |
| return set() |
🤖 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/ParameterManager.py` around lines 80 - 83, The
get_bool_param_pairs method currently calls st.session_state.get(...) which
throws RuntimeError outside of a Streamlit session; guard this call the same way
as _merge_bool_params_from_ini does (e.g., check for an active session or catch
RuntimeError) and return the default empty set when no session exists; update
get_bool_param_pairs to first verify Streamlit session availability before
accessing st.session_state.get using the same check/pattern used in
_merge_bool_params_from_ini so behavior is consistent.
| sk = self.bool_pairs_session_key() | ||
| if sk not in st.session_state: | ||
| st.session_state[sk] = set() | ||
| for short in bool_param_paths_from_param_xml_ini(ini_path, tool): | ||
| st.session_state[sk].add((tool, short)) |
There was a problem hiding this comment.
Replace per-tool bool metadata instead of only unioning.
Lines 93-94 only add entries. If a tool .ini changes during a session, removed bool params remain cached and may be emitted incorrectly as flags.
🔧 Suggested change
sk = self.bool_pairs_session_key()
if sk not in st.session_state:
st.session_state[sk] = set()
+ st.session_state[sk] = {
+ pair for pair in st.session_state[sk] if pair[0] != tool
+ }
for short in bool_param_paths_from_param_xml_ini(ini_path, tool):
st.session_state[sk].add((tool, short))🤖 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/ParameterManager.py` around lines 90 - 94, The code currently
unions new bool params into the session set (using bool_pairs_session_key())
causing stale params to persist when a tool's .ini removes flags; instead,
compute the current set of bool pairs for the given tool by calling
bool_param_paths_from_param_xml_ini(ini_path, tool), remove any existing entries
in st.session_state[sk] that belong to this tool (matching the first element of
the tuple), then add the current tuples (tool, short) or simply replace
st.session_state[sk] entries for that tool with the new ones; update the logic
around bool_pairs_session_key(), bool_param_paths_from_param_xml_ini, ini_path
and tool to ensure removal of old tool-specific entries before adding the fresh
list.
1 participant
Changes
Tool instance-aware parameter lookup
tool_instance_nameparameter toCommandExecutor.run_toppparams.jsonusing instance-specific keysBoolean parameter handling
bool_param_paths_from_param_xml_ini()utility to parse boolean parameters from TOPP.inifiles-flag) instead of-flag trueParameterManager improvements
.inifilescreate_ini()now automatically registers bool parameter metadataSummary by CodeRabbit
New Features
Chores