Add collapsible sections to tool parameters #306
Conversation
Add a new `conditional_sections` parameter to `input_TOPP()` that allows
parameter sections to be shown/hidden based on a controlling parameter's value:
- Sections are hidden entirely when the controlling parameter doesn't match
- Sections are shown in an expanded expander when the value matches
- Supports single values or lists of values for matching
- Validates that controlling parameters are not inside their controlled sections
- Supports prefix matching (e.g., "algorithm" controls "algorithm:common")
Example usage:
conditional_sections={
"algorithm:mtd": {
"param": "algorithm:common:enable_mtd",
"value": "true"
}
}
📝 WalkthroughWalkthroughThe changes introduce conditional section visibility control to TOPP parameter input handling. A new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🪛 Ruff (0.14.11)src/workflow/StreamlitUI.py545-545: Do not use mutable data structures for argument defaults Replace with (B006) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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 PR adds a new conditional_sections parameter to the input_TOPP() function that enables dynamic visibility control of parameter sections based on the values of controlling parameters. Sections can be hidden when conditions aren't met or shown in an expanded expander when they are.
Changes:
- Added
conditional_sectionsparameter toinput_TOPP()with validation logic to prevent circular dependencies - Implemented section visibility logic with prefix matching support for hierarchical sections
- Added helper functions for parameter value retrieval, section control checking, and label formatting
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/workflow/StreamlitUI.py | Added conditional sections feature with validation, visibility control logic, and expander display for controlled sections |
| src/Workflow.py | Added commented example demonstrating usage of the new conditional_sections parameter |
Comments suppressed due to low confidence (1)
src/workflow/StreamlitUI.py:659
- Corrected spelling of 'adavnaced' to 'advanced'.
# Skip adavnaced parameters if not selected
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_param_value(param_name: str): | ||
| """Get the current value of a parameter by its short name (e.g., 'algorithm:common:param').""" | ||
| for p in params: | ||
| short_name = p["key"].decode().split(":1:")[1] | ||
| if short_name == param_name: | ||
| return p["value"] |
There was a problem hiding this comment.
The get_param_value function performs a linear search through all parameters each time it's called. This is invoked for every section via should_show_section(). Consider building a parameter lookup dictionary once before the loop to avoid O(n) lookups for each section, especially when there are many sections.
| def get_param_value(param_name: str): | |
| """Get the current value of a parameter by its short name (e.g., 'algorithm:common:param').""" | |
| for p in params: | |
| short_name = p["key"].decode().split(":1:")[1] | |
| if short_name == param_name: | |
| return p["value"] | |
| # Build a lookup table for parameters by their short name to avoid | |
| # repeatedly scanning the full `params` list in get_param_value(). | |
| param_lookup: dict[str, dict] = {} | |
| for _p in params: | |
| short_name = _p["key"].decode().split(":1:")[1] | |
| # Preserve previous behavior of returning the first matching entry | |
| # in case of duplicate short names. | |
| if short_name not in param_lookup: | |
| param_lookup[short_name] = _p | |
| def get_param_value(param_name: str): | |
| """Get the current value of a parameter by its short name (e.g., 'algorithm:common:param').""" | |
| p = param_lookup.get(param_name) | |
| if p is not None: | |
| return p["value"] |
| # Check prefix matches (e.g., "algorithm" controls "algorithm:common") | ||
| for controlled_section in conditional_sections: |
There was a problem hiding this comment.
The prefix matching logic may return the wrong control config when multiple controlled sections share a common prefix. For example, if conditional_sections contains both "algorithm" and "algorithm:common", checking "algorithm:common:param" could match "algorithm" first depending on dictionary iteration order. Consider sorting controlled sections by length in descending order before checking, or use more specific matching logic.
| # Check prefix matches (e.g., "algorithm" controls "algorithm:common") | |
| for controlled_section in conditional_sections: | |
| # Check prefix matches (e.g., "algorithm" controls "algorithm:common"). | |
| # When multiple keys share a prefix (e.g., "algorithm" and "algorithm:common"), | |
| # prefer the most specific (longest) match by sorting keys by length descending. | |
| for controlled_section in sorted(conditional_sections, key=len, reverse=True): |
| # Validate that controlling parameters are not inside their controlled sections | ||
| if conditional_sections: | ||
| for controlled_section, control in conditional_sections.items(): | ||
| controlling_param = control.get("param", "") |
There was a problem hiding this comment.
If the controlling parameter name is empty (when "param" key is missing from the control dict), controlling_param_section will also be empty, and the validation check on lines 771-772 may not detect all invalid configurations. Consider adding validation to ensure both "param" and "value" keys exist in control configs before proceeding.
| controlling_param = control.get("param", "") | |
| # Ensure control config contains required keys | |
| if not isinstance(control, dict) or "param" not in control or "value" not in control: | |
| st.error( | |
| f"Configuration error: Conditional section '{controlled_section}' " | |
| "must define both 'param' and 'value' keys." | |
| ) | |
| return | |
| controlling_param = control.get("param") | |
| # controlling_param must be a non-empty string | |
| if not isinstance(controlling_param, str) or not controlling_param.strip(): | |
| st.error( | |
| f"Configuration error: Controlling parameter for section " | |
| f"'{controlled_section}' must be a non-empty string." | |
| ) | |
| return |
|
|
||
| # Check if current value matches any expected value | ||
| if current_value in expected_values: | ||
| return (True, True) # Show in expander | ||
| # Handle string comparison for booleans |
There was a problem hiding this comment.
When current_value is None (parameter not found), the function still converts it to string "none" and checks against expected values. This could lead to unintended matches if "none" or "None" is in the expected values list. Consider explicitly handling the None case before performing comparisons.
| # Check if current value matches any expected value | |
| if current_value in expected_values: | |
| return (True, True) # Show in expander | |
| # Handle string comparison for booleans | |
| # If the controlling parameter is not found (None), do not match any expected value | |
| if current_value is None: | |
| return (False, False) # Hide entirely when controlling parameter is missing | |
| # Check if current value matches any expected value | |
| if current_value in expected_values: | |
| return (True, True) # Show in expander | |
| # Handle string comparison for booleans and case-insensitive matches |
| ) | ||
| return | ||
|
|
||
| def display_TOPP_params(params: dict, num_cols): |
There was a problem hiding this comment.
The parameter params is annotated as dict but is actually a list of parameter dictionaries based on how it's called (e.g., section_params on line 878). The type annotation should be list instead of dict.
| def display_TOPP_params(params: dict, num_cols): | |
| def display_TOPP_params(params: list[dict[str, Any]], num_cols): |
| if not section or section == "all": | ||
| return "Parameters" | ||
| parts = section.split(":") | ||
| return ":".join(parts[:-1]) + (":" if len(parts) > 1 else "") + f"**{parts[-1]}**" |
There was a problem hiding this comment.
The label formatting logic creates markdown with bold formatting for the last part. However, when there's only one part (len(parts) == 1), it returns **part** without any prefix, but when there are multiple parts, it adds a trailing colon before the bold part. This creates inconsistent formatting. Consider simplifying to always return the formatted string consistently regardless of the number of parts.
| return ":".join(parts[:-1]) + (":" if len(parts) > 1 else "") + f"**{parts[-1]}**" | |
| if len(parts) == 1: | |
| # Single-part section: just bold the name | |
| return f"**{parts[0]}**" | |
| # Multi-part section: show prefix, then bold the last part | |
| prefix = ":".join(parts[:-1]) | |
| return f"{prefix}:**{parts[-1]}**" |
| for section, section_params in param_sections.items(): | ||
| should_show, use_expander = should_show_section(section) | ||
|
|
||
| if not should_show: | ||
| continue # Hide section entirely |
There was a problem hiding this comment.
The new conditional section visibility logic lacks test coverage. The feature includes multiple code paths (exact match, prefix match, value matching, validation errors) that should be tested to ensure correct behavior, especially the validation logic on lines 765-777 and the section visibility decisions.
Add a new
conditional_sectionsparameter toinput_TOPP()that allows parameter sections to be shown/hidden based on a controlling parameter's value:Example usage:
conditional_sections={ "algorithm:mtd": { "param": "algorithm:common:enable_mtd", "value": "true" } }
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.