add support for empty list values#311
Conversation
📝 WalkthroughWalkthroughThe changes improve parameter handling in TOPP tool execution by filtering empty values when building commands and extending default parameter detection to account for empty list defaults during parameter persistence. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for handling empty list values in parameter management. The changes ensure that empty list parameters are saved and properly passed to command-line tools.
Changes:
- Modified parameter saving logic to detect and persist empty list parameters
- Updated command construction to handle empty parameter values by passing flags without values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/workflow/ParameterManager.py | Added logic to detect list parameters and save them even when empty |
| src/workflow/CommandExecutor.py | Modified command building to skip adding values for empty parameters (pass flag only) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (ini_value != value) | ||
| (ini_value != value) | ||
| or (key.split(":1:")[1] in json_params[tool]) | ||
| or (is_list_param and not value) # Always save empty list params |
There was a problem hiding this comment.
The condition not value will evaluate to True for many falsy values beyond empty lists (e.g., empty strings, 0, False, None). This could cause unintended parameters to be saved. Consider using a more explicit check like is_list_param and isinstance(value, list) and len(value) == 0 to specifically target empty lists.
| or (is_list_param and not value) # Always save empty list params | |
| or (is_list_param and isinstance(value, list) and len(value) == 0) # Always save empty list params |
| command += v.split("\n") | ||
| else: | ||
| command += [str(v)] | ||
| if v: # Skip empty values (pass flag with no value) |
There was a problem hiding this comment.
The condition if v: uses Python's truthiness test, which will skip not only empty lists but also other falsy values like empty strings, 0, False, or None. This may cause unintended behavior where legitimate parameter values are skipped. Consider using a more explicit check like if v is not None and (not isinstance(v, (list, str)) or v): to handle empty collections specifically while preserving other falsy values like 0 or False that may be valid parameter values.
| is_list_param = isinstance(ini_value, list) | ||
| # check if value is different from default OR is an empty list parameter | ||
| if ( | ||
| (ini_value != value) | ||
| (ini_value != value) | ||
| or (key.split(":1:")[1] in json_params[tool]) | ||
| or (is_list_param and not value) # Always save empty list params |
There was a problem hiding this comment.
The new behavior for handling empty list parameters lacks test coverage. Consider adding test cases that verify: (1) empty list parameters are correctly saved in ParameterManager.save_parameters(), (2) empty list parameters result in command flags without values in CommandExecutor.run_topp(), and (3) the interaction between the two components works correctly for empty list scenarios.
| is_list_param = isinstance(ini_value, list) | ||
| # check if value is different from default OR is an empty list parameter | ||
| if ( | ||
| (ini_value != value) | ||
| (ini_value != value) | ||
| or (key.split(":1:")[1] in json_params[tool]) | ||
| or (is_list_param and not value) # Always save empty list params |
There was a problem hiding this comment.
The docstring for save_parameters() states it ensures "only non-default values are stored", but the new behavior now also stores empty list parameters even when they match the default. Consider updating the docstring to reflect this new behavior, e.g., "ensuring that only non-default values and empty list parameters are stored".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/workflow/CommandExecutor.py`:
- Around line 220-225: The parameter-value check currently uses a truthy test
(`if v:`) which drops valid values like 0 and False; update the logic in the
command construction loop that appends flags and values (the block that appends
f"-{k}" and then tests v) to instead treat only None, empty string (""), and
empty list ([]) as "no value" — i.e., append the flag and then if v is not None
and v != "" and v != [] then append the value(s) (splitting on "\n" when v is a
string); make the identical change in the custom_params loop so both TOPP tool
params and custom params preserve numeric and boolean values.
🧹 Nitpick comments (1)
src/workflow/ParameterManager.py (1)
69-75: Broaden list-type detection for pyopenms Param values.
Param.getValuemay return non-listsequence types (e.g.,StringList/IntListwrappers). If so,isinstance(ini_value, list)won’t detect list defaults and empty-list persistence will be skipped. Please verify the return type in pyopenms 3.3.0 and consider a more general sequence check.♻️ Possible adjustment
+from collections.abc import Sequence ... - is_list_param = isinstance(ini_value, list) + is_list_param = isinstance(ini_value, Sequence) and not isinstance(ini_value, (str, bytes))
| command += [f"-{k}"] | ||
| if isinstance(v, str) and "\n" in v: | ||
| command += v.split("\n") | ||
| else: | ||
| command += [str(v)] | ||
| if v: # Skip empty values (pass flag with no value) | ||
| if isinstance(v, str) and "\n" in v: | ||
| command += v.split("\n") | ||
| else: | ||
| command += [str(v)] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CommandExecutor.py" -type fRepository: OpenMS/streamlit-template
Length of output: 102
🏁 Script executed:
head -250 src/workflow/CommandExecutor.py | tail -50Repository: OpenMS/streamlit-template
Length of output: 2265
🏁 Script executed:
wc -l src/workflow/CommandExecutor.pyRepository: OpenMS/streamlit-template
Length of output: 104
🏁 Script executed:
grep -n "params\[" src/workflow/CommandExecutor.py | head -20Repository: OpenMS/streamlit-template
Length of output: 122
🏁 Script executed:
grep -n "def " src/workflow/CommandExecutor.py | head -20Repository: OpenMS/streamlit-template
Length of output: 597
🏁 Script executed:
grep -n "\.items()" src/workflow/CommandExecutor.pyRepository: OpenMS/streamlit-template
Length of output: 387
🏁 Script executed:
rg -l "params\[" --type py | head -10Repository: OpenMS/streamlit-template
Length of output: 321
🏁 Script executed:
sed -n '160,245p' src/workflow/CommandExecutor.pyRepository: OpenMS/streamlit-template
Length of output: 4367
🏁 Script executed:
rg -n "class ParameterManager" --type pyRepository: OpenMS/streamlit-template
Length of output: 127
🏁 Script executed:
head -100 src/workflow/ParameterManager.pyRepository: OpenMS/streamlit-template
Length of output: 4626
🏁 Script executed:
rg "get_parameters_from_json" -A 5 -B 5Repository: OpenMS/streamlit-template
Length of output: 5797
🏁 Script executed:
grep -r "if v:" --type pyRepository: OpenMS/streamlit-template
Length of output: 186
🏁 Script executed:
grep -rn "\.split\(" src/workflow/CommandExecutor.pyRepository: OpenMS/streamlit-template
Length of output: 92
🏁 Script executed:
rg "if v:" src/workflow/CommandExecutor.py -B 2 -A 4Repository: OpenMS/streamlit-template
Length of output: 702
🏁 Script executed:
rg "isinstance.*str.*split" src/workflow/CommandExecutor.py -B 2 -A 2Repository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
grep -n "params\[tool\]" src/workflow/CommandExecutor.pyRepository: OpenMS/streamlit-template
Length of output: 122
🏁 Script executed:
find . -name "*.ini" -type f | head -5Repository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
rg "param" docs/ | head -20Repository: OpenMS/streamlit-template
Length of output: 3994
🏁 Script executed:
rg "boolean\|bool\|numeric\|0\|False" README.mdRepository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
find . -name "*.py" -type f -exec grep -l "run_topp" {} \;Repository: OpenMS/streamlit-template
Length of output: 144
🏁 Script executed:
rg "run_topp" -A 10 -B 2 | head -50Repository: OpenMS/streamlit-template
Length of output: 3704
🏁 Script executed:
cat src/Workflow.py | head -150Repository: OpenMS/streamlit-template
Length of output: 5073
🏁 Script executed:
rg "Value\|value" docs/toppframework.py | head -20Repository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
find . -name "example.py" -o -name "*example*" -type f | grep python-toolsRepository: OpenMS/streamlit-template
Length of output: 98
🏁 Script executed:
ls -la src/python-tools/ 2>/dev/null || echo "Directory not found"Repository: OpenMS/streamlit-template
Length of output: 319
🏁 Script executed:
cat src/python-tools/example.pyRepository: OpenMS/streamlit-template
Length of output: 2074
🏁 Script executed:
rg "Param()" src/ -A 5 -B 5Repository: OpenMS/streamlit-template
Length of output: 12212
🏁 Script executed:
rg "getValue\|setValue" src/ -B 3 -A 3 | head -40Repository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
rg "0\|False" src/Workflow.pyRepository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
cat src/workflow/ParameterManager.py | grep -A 20 "Always save empty"Repository: OpenMS/streamlit-template
Length of output: 958
🏁 Script executed:
rg "algorithm:common" src/ -B 2 -A 2Repository: OpenMS/streamlit-template
Length of output: 334
🏁 Script executed:
rg "noise_threshold_int" . --type pyRepository: OpenMS/streamlit-template
Length of output: 168
🏁 Script executed:
rg "setdefault\|get\|pop" src/workflow/CommandExecutor.pyRepository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
cat src/workflow/ParameterManager.py | sed -n '40,80p'Repository: OpenMS/streamlit-template
Length of output: 2043
🏁 Script executed:
rg "value.*0\|value.*False\|\"0\"\|False" src/Workflow.py src/python-tools/Repository: OpenMS/streamlit-template
Length of output: 51
🏁 Script executed:
cat src/workflow/CommandExecutor.py | sed -n '215,235p'Repository: OpenMS/streamlit-template
Length of output: 926
🏁 Script executed:
rg "\"value\":" src/python-tools/example.py -A 2Repository: OpenMS/streamlit-template
Length of output: 580
🏁 Script executed:
web_search
OpenMS TOPP tools command line parameter format boolean integer valuesRepository: OpenMS/streamlit-template
Length of output: 164
🌐 Web query:
OpenMS TOPP tools command line parameters boolean 0 False integer values
💡 Result:
Short answer: TOPP boolean parameters accept either true/false (case-insensitive) or 1/0; integer parameters are plain integers and may have documented ranges. Use the same name on the CLI or in INI files (e.g. -no_progress true or no_progress=1 in an .ini). Example docs show boolean options listed as "true, false" and integer options with defaults and ranges like "threads 1" or "index 0 (0:∞)". [1][2]
Sources:
- TOPP tools command‑line & INI overview. [1]
- TOPP tool parameter pages showing booleans ("true, false") and integers with defaults/ranges (e.g. threads, index, no_progress). [2]
Replace if v: with explicit empty-value checks to preserve 0 and False parameters.
The current check if v: treats 0 and False as empty, causing valid numeric and boolean parameter values to be dropped from the command. TOPP tools expect -param value pairs; when the value is omitted, the command is malformed. This affects both TOPP tool parameters (lines 219–225) and custom parameters (lines 227–233).
Replace with explicit checks for None, empty strings, and empty lists:
🐛 Suggested fix
for k, v in params[tool].items():
command += [f"-{k}"]
- if v: # Skip empty values (pass flag with no value)
+ if v is not None and v != "" and v != []:
if isinstance(v, str) and "\n" in v:
command += v.split("\n")
else:
command += [str(v)]Apply the same fix to the custom_params loop (lines 227–233).
📝 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.
| command += [f"-{k}"] | |
| if isinstance(v, str) and "\n" in v: | |
| command += v.split("\n") | |
| else: | |
| command += [str(v)] | |
| if v: # Skip empty values (pass flag with no value) | |
| if isinstance(v, str) and "\n" in v: | |
| command += v.split("\n") | |
| else: | |
| command += [str(v)] | |
| command += [f"-{k}"] | |
| if v is not None and v != "" and v != []: | |
| if isinstance(v, str) and "\n" in v: | |
| command += v.split("\n") | |
| else: | |
| command += [str(v)] |
🤖 Prompt for AI Agents
In `@src/workflow/CommandExecutor.py` around lines 220 - 225, The parameter-value
check currently uses a truthy test (`if v:`) which drops valid values like 0 and
False; update the logic in the command construction loop that appends flags and
values (the block that appends f"-{k}" and then tests v) to instead treat only
None, empty string (""), and empty list ([]) as "no value" — i.e., append the
flag and then if v is not None and v != "" and v != [] then append the value(s)
(splitting on "\n" when v is a string); make the identical change in the
custom_params loop so both TOPP tool params and custom params preserve numeric
and boolean values.
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.