From 0fd9cec7ea6d918bae40625d8a1f88007f2e2e29 Mon Sep 17 00:00:00 2001 From: "Sanjith Badri K." Date: Thu, 12 Mar 2026 20:59:40 +0530 Subject: [PATCH 1/6] fix: correctly parse and route implicit boolean flags from ini files (#90) --- src/workflow/CommandExecutor.py | 43 +++++++++++++++++++++----------- src/workflow/ParameterManager.py | 32 +++++++++++++++++++++++- 2 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/workflow/CommandExecutor.py b/src/workflow/CommandExecutor.py index 86f265b2..838173b5 100644 --- a/src/workflow/CommandExecutor.py +++ b/src/workflow/CommandExecutor.py @@ -263,6 +263,10 @@ def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}) -> b # Load parameters for non-defaults params = self.parameter_manager.get_parameters_from_json() + + # NEW LOGIC: Ask the ParameterManager which keys are strictly booleans + bool_params = self.parameter_manager.get_boolean_parameters(tool) + # Construct commands for each process for i in range(n_processes): command = [tool] @@ -281,27 +285,38 @@ def run_topp(self, tool: str, input_output: dict, custom_params: dict = {}) -> b # standard case, files was a list of strings, take the file name at index else: command += [value[i]] + # Add non-default TOPP tool parameters if tool in params.keys(): for k, v in params[tool].items(): + # NEW LOGIC: Intercept boolean flags + if k in bool_params: + # Only add the implicit flag if True. If False, omit entirely. + if str(v).lower() == "true": + command += [f"-{k}"] + else: + # Existing logic for strings, ints, floats + command += [f"-{k}"] + if v != "" and v is not None: + if isinstance(v, str) and "\n" in v: + command += v.split("\n") + else: + command += [str(v)] + + # Add custom parameters + for k, v in custom_params.items(): + # NEW LOGIC: Intercept custom boolean flags + if k in bool_params: + if str(v).lower() == "true": + command += [f"-{k}"] + else: command += [f"-{k}"] - # Skip only empty strings (pass flag with no value) - # Note: 0 and 0.0 are valid values, so use explicit check if v != "" and v is not None: - if isinstance(v, str) and "\n" in v: - command += v.split("\n") + if isinstance(v, list): + command += [str(x) for x in v] else: command += [str(v)] - # Add custom parameters - for k, v in custom_params.items(): - command += [f"-{k}"] - # Skip only empty strings (pass flag with no value) - # Note: 0 and 0.0 are valid values, so use explicit check - if v != "" and v is not None: - if isinstance(v, list): - command += [str(x) for x in v] - else: - command += [str(v)] + # Add threads parameter for TOPP tools command += ["-threads", str(threads_per_command)] commands.append(command) diff --git a/src/workflow/ParameterManager.py b/src/workflow/ParameterManager.py index b0c36263..659256d7 100644 --- a/src/workflow/ParameterManager.py +++ b/src/workflow/ParameterManager.py @@ -3,6 +3,7 @@ import shutil import subprocess import streamlit as st +import xml.etree.ElementTree as ET from pathlib import Path class ParameterManager: @@ -287,4 +288,33 @@ def clear_parameter_session_state(self) -> None: if key.startswith(self.param_prefix) or key.startswith(self.topp_param_prefix) ] for key in keys_to_delete: - del st.session_state[key] \ No newline at end of file + del st.session_state[key] + + def get_boolean_parameters(self, tool: str) -> list: + """ + Parses the tool's .ini XML file to find all parameters explicitly defined as type 'bool'. + This bypasses the pyOpenMS internal mapping which loses the boolean type definition. + + Args: + tool: Name of the TOPP tool (e.g., "FeatureFinderMetabo") + + Returns: + list: A list of parameter names (strings) that are strictly booleans. + """ + ini_path = Path(self.ini_dir, f"{tool}.ini") + if not ini_path.exists(): + return [] + + bool_params = [] + try: + tree = ET.parse(ini_path) + root = tree.getroot() + # Recursively find all ITEM tags in the XML tree with type="bool" + for item in root.findall(".//ITEM[@type='bool']"): + name = item.get("name") + if name: + bool_params.append(name) + except Exception: + pass # Safely return empty list if XML parsing fails + + return bool_params \ No newline at end of file From 8cd956ca3c64c887910720d7b03048cc6f67f7fa Mon Sep 17 00:00:00 2001 From: "Sanjith Badri K." Date: Thu, 12 Mar 2026 21:38:34 +0530 Subject: [PATCH 2/6] fix: force ini creation before parsing to prevent fallback --- src/workflow/ParameterManager.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/workflow/ParameterManager.py b/src/workflow/ParameterManager.py index 659256d7..715bbc4f 100644 --- a/src/workflow/ParameterManager.py +++ b/src/workflow/ParameterManager.py @@ -301,10 +301,12 @@ def get_boolean_parameters(self, tool: str) -> list: Returns: list: A list of parameter names (strings) that are strictly booleans. """ - ini_path = Path(self.ini_dir, f"{tool}.ini") - if not ini_path.exists(): + # SELF-HEALING FIX: Force generate the .ini file if it doesn't exist yet + if not self.create_ini(tool): return [] + ini_path = Path(self.ini_dir, f"{tool}.ini") + bool_params = [] try: tree = ET.parse(ini_path) From 255a2e91538a4eacea1419c27d72c94d2c814039 Mon Sep 17 00:00:00 2001 From: "Sanjith Badri K." Date: Thu, 12 Mar 2026 22:30:18 +0530 Subject: [PATCH 3/6] fix: recursively parse XML to capture full hierarchical parameter paths --- src/workflow/ParameterManager.py | 39 +++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/workflow/ParameterManager.py b/src/workflow/ParameterManager.py index 715bbc4f..68d8136b 100644 --- a/src/workflow/ParameterManager.py +++ b/src/workflow/ParameterManager.py @@ -299,7 +299,8 @@ def get_boolean_parameters(self, tool: str) -> list: tool: Name of the TOPP tool (e.g., "FeatureFinderMetabo") Returns: - list: A list of parameter names (strings) that are strictly booleans. + list: A list of parameter names (strings) that are strictly booleans, + including their full hierarchical paths (e.g., 'algorithm:epd:masstrace_snr_filtering'). """ # SELF-HEALING FIX: Force generate the .ini file if it doesn't exist yet if not self.create_ini(tool): @@ -311,12 +312,34 @@ def get_boolean_parameters(self, tool: str) -> list: try: tree = ET.parse(ini_path) root = tree.getroot() - # Recursively find all ITEM tags in the XML tree with type="bool" - for item in root.findall(".//ITEM[@type='bool']"): - name = item.get("name") - if name: - bool_params.append(name) + + # Recursive function to build the hierarchical path from XML nodes + def traverse(node, current_path): + for child in node: + if child.tag == "ITEM" and child.get("type") == "bool": + name = child.get("name") + if name: + bool_params.append(current_path + name) + elif child.tag == "NODE": + name = child.get("name") + if name: + # Append the node name and a colon to the path + traverse(child, current_path + name + ":") + + # Start traversal from the XML root + traverse(root, "") + + # OpenMS INI files usually encapsulate everything in a top-level . + # We must strip this prefix so the keys perfectly match the JSON session state keys. + tool_prefix = f"{tool}:" + cleaned_params = [ + p[len(tool_prefix):] if p.startswith(tool_prefix) else p + for p in bool_params + ] + + return cleaned_params + except Exception: - pass # Safely return empty list if XML parsing fails + pass # Safely return empty list if XML parsing fails - return bool_params \ No newline at end of file + return [] \ No newline at end of file From f681655a5af8a0a7bd10d2ee8f736bd3133dff70 Mon Sep 17 00:00:00 2001 From: "Sanjith Badri K." Date: Thu, 12 Mar 2026 22:44:42 +0530 Subject: [PATCH 4/6] fix: include instance node '1:' in prefix stripping and add error logging --- src/workflow/ParameterManager.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/workflow/ParameterManager.py b/src/workflow/ParameterManager.py index 68d8136b..502c53e2 100644 --- a/src/workflow/ParameterManager.py +++ b/src/workflow/ParameterManager.py @@ -331,7 +331,7 @@ def traverse(node, current_path): # OpenMS INI files usually encapsulate everything in a top-level . # We must strip this prefix so the keys perfectly match the JSON session state keys. - tool_prefix = f"{tool}:" + tool_prefix = f"{tool}:1:" cleaned_params = [ p[len(tool_prefix):] if p.startswith(tool_prefix) else p for p in bool_params @@ -339,7 +339,7 @@ def traverse(node, current_path): return cleaned_params - except Exception: + except Exception as e: + print(f"Error parsing boolean parameters for {tool}: {e}") pass # Safely return empty list if XML parsing fails - return [] \ No newline at end of file From 3d48c579c1cd1395062a25d49eb317e7e911e404 Mon Sep 17 00:00:00 2001 From: "Sanjith Badri K." Date: Sun, 15 Mar 2026 20:01:55 +0530 Subject: [PATCH 5/6] fix: address CodeRabbit feedback (defusedxml, strict exceptions, docstrings) --- requirements.txt | 1 + src/workflow/ParameterManager.py | 27 ++++++++++++++++++--------- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/requirements.txt b/requirements.txt index 938ccc1e..95e72be5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,6 +26,7 @@ contourpy==1.3.1 # via matplotlib cycler==0.12.1 # via matplotlib +defusedxml==0.7.1 fonttools==4.59.2 # via matplotlib gitdb==4.0.12 diff --git a/src/workflow/ParameterManager.py b/src/workflow/ParameterManager.py index 502c53e2..e6dc9eb8 100644 --- a/src/workflow/ParameterManager.py +++ b/src/workflow/ParameterManager.py @@ -3,7 +3,7 @@ import shutil import subprocess import streamlit as st -import xml.etree.ElementTree as ET +import defusedxml.ElementTree as ET from pathlib import Path class ParameterManager: @@ -292,24 +292,32 @@ def clear_parameter_session_state(self) -> None: def get_boolean_parameters(self, tool: str) -> list: """ - Parses the tool's .ini XML file to find all parameters explicitly defined as type 'bool'. - This bypasses the pyOpenMS internal mapping which loses the boolean type definition. + Parses the tool's generated .ini (XML) file to discover strictly boolean parameters. + This prevents implicit booleans from being passed as strings to the command line. Args: - tool: Name of the TOPP tool (e.g., "FeatureFinderMetabo") + tool (str): The name of the TOPP tool (e.g., 'FeatureFinderMetabo'). Returns: - list: A list of parameter names (strings) that are strictly booleans, - including their full hierarchical paths (e.g., 'algorithm:epd:masstrace_snr_filtering'). + list: A list of hierarchical parameter keys that are explicitly typed as 'bool'. + + Raises: + FileNotFoundError: If the .ini file does not exist. + RuntimeError: If the .ini file fails to generate. + ET.ParseError: If the XML parsing fails. """ - # SELF-HEALING FIX: Force generate the .ini file if it doesn't exist yet if not self.create_ini(tool): - return [] + # CodeRabbit Fix: Raise an explicit error instead of silently returning [] + raise RuntimeError(f"Failed to generate .ini file for TOPP tool: {tool}") ini_path = Path(self.ini_dir, f"{tool}.ini") + if not ini_path.exists(): + # CodeRabbit Fix: Raise an explicit error instead of silently returning [] + raise FileNotFoundError(f"Missing expected .ini file for TOPP tool at: {ini_path}") bool_params = [] try: + # CodeRabbit Fix: Use defusedxml (imported as ET) to safely parse the file tree = ET.parse(ini_path) root = tree.getroot() @@ -339,7 +347,8 @@ def traverse(node, current_path): return cleaned_params - except Exception as e: + except ET.ParseError as e: + logger.error(f"XML parsing failed for {tool}: {e}"); raise print(f"Error parsing boolean parameters for {tool}: {e}") pass # Safely return empty list if XML parsing fails return [] \ No newline at end of file From 4f2f6ec2c21cacfc78655968206a4ea14e0ff962 Mon Sep 17 00:00:00 2001 From: "Sanjith Badri K." Date: Sun, 15 Mar 2026 20:14:26 +0530 Subject: [PATCH 6/6] fix: address CodeRabbit feedback (defusedxml, strict exceptions, docstrings) --- src/workflow/ParameterManager.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/workflow/ParameterManager.py b/src/workflow/ParameterManager.py index e6dc9eb8..2aa7e3ac 100644 --- a/src/workflow/ParameterManager.py +++ b/src/workflow/ParameterManager.py @@ -347,8 +347,9 @@ def traverse(node, current_path): return cleaned_params - except ET.ParseError as e: - logger.error(f"XML parsing failed for {tool}: {e}"); raise + except ET.ParseError as e: + st.error(f"XML parsing failed for {tool}: {e}") + raise print(f"Error parsing boolean parameters for {tool}: {e}") pass # Safely return empty list if XML parsing fails return [] \ No newline at end of file