-
Notifications
You must be signed in to change notification settings - Fork 35
support flag parameters without valuesAllow flags to work with only key names, omitting values. #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
hjn0415a
wants to merge
1
commit into
OpenMS:main
Choose a base branch
from
hjn0415a:feature/flag-parameter
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+96
−3
Open
support flag parameters without valuesAllow flags to work with only key names, omitting values. #391
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,3 +13,4 @@ gdpr_consent/node_modules/ | |
| *~ | ||
| .streamlit/secrets.toml | ||
| docs/superpowers/ | ||
| .venv/ | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,52 @@ | |
| import shutil | ||
| import subprocess | ||
| import streamlit as st | ||
| import xml.etree.ElementTree as ET | ||
| from pathlib import Path | ||
|
|
||
|
|
||
| def bool_param_paths_from_param_xml_ini(ini_path: Path, tool_stem: str) -> set[str]: | ||
| """ | ||
| Return short parameter paths for every ``<ITEM type="bool">`` in a ParamXML .ini file. | ||
|
|
||
| Paths match the suffix after ``Tool:1:`` in pyOpenMS (e.g. ``algorithm:epd:masstrace_snr_filtering``). | ||
| """ | ||
| try: | ||
| root = ET.parse(ini_path).getroot() | ||
| except (ET.ParseError, OSError): | ||
| return set() | ||
|
|
||
| def local_tag(el: ET.Element) -> str: | ||
| t = el.tag | ||
| return t.rsplit("}", 1)[-1] if isinstance(t, str) and "}" in t else str(t) | ||
|
|
||
| out: set[str] = set() | ||
|
|
||
| def walk(el: ET.Element, parts: tuple[str, ...]) -> None: | ||
| for ch in el: | ||
| lt = local_tag(ch) | ||
| if lt == "NODE": | ||
| nm = ch.get("name") or "" | ||
| walk(ch, parts + (nm,)) | ||
| elif lt == "ITEM" and (ch.get("type") or "").lower() == "bool": | ||
| nm = ch.get("name") or "" | ||
| segs = [p for p in parts if p] | ||
| if nm: | ||
| segs.append(nm) | ||
| if not segs: | ||
| continue | ||
| # Strip tool root NODE name and instance NODE "1" (not part of pyOpenMS short keys) | ||
| while segs and segs[0] in (tool_stem, "1"): | ||
| segs.pop(0) | ||
| if segs: | ||
| out.add(":".join(segs)) | ||
|
|
||
| for ch in root: | ||
| if local_tag(ch) == "NODE": | ||
| walk(ch, ()) | ||
| return out | ||
|
|
||
|
|
||
| class ParameterManager: | ||
| """ | ||
| Manages the parameters for a workflow, including saving parameters to a JSON file, | ||
|
|
@@ -29,6 +73,29 @@ def __init__(self, workflow_dir: Path, workflow_name: str = None): | |
| # Store workflow name for preset loading; default to directory stem if not provided | ||
| self.workflow_name = workflow_name or workflow_dir.stem | ||
|
|
||
| def bool_pairs_session_key(self) -> str: | ||
| """Session state key holding a set of (tool name, param path) for bool TOPP params.""" | ||
| return f"{self.ini_dir.parent.stem}-topp-bool-pairs" | ||
|
|
||
| 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 _merge_bool_params_from_ini(self, tool: str) -> None: | ||
| """Load tool.ini (XML) and merge type=bool parameter paths into session_state.""" | ||
| ini_path = Path(self.ini_dir, f"{tool}.ini") | ||
| if not ini_path.exists(): | ||
| return | ||
| try: | ||
| 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)) | ||
|
Comment on lines
+90
to
+94
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replace per-tool bool metadata instead of only unioning. Lines 93-94 only add entries. If a tool 🔧 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 |
||
| except RuntimeError: | ||
| # No Streamlit session (e.g. plain `python` import) | ||
| pass | ||
|
|
||
| def create_ini(self, tool: str) -> bool: | ||
| """ | ||
| Create an ini file for a TOPP tool if it doesn't exist. | ||
|
|
@@ -41,11 +108,14 @@ def create_ini(self, tool: str) -> bool: | |
| """ | ||
| ini_path = Path(self.ini_dir, tool + ".ini") | ||
| if ini_path.exists(): | ||
| self._merge_bool_params_from_ini(tool) | ||
| return True | ||
| try: | ||
| subprocess.call([tool, "-write_ini", str(ini_path)]) | ||
| except FileNotFoundError: | ||
| return False | ||
| if ini_path.exists(): | ||
| self._merge_bool_params_from_ini(tool) | ||
| return ini_path.exists() | ||
|
|
||
| def save_parameters(self) -> None: | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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:
Repository: 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 accessst.session_stateoutside an active Streamlit runtime session. Most common causes 1) Running the file as a normal Python script -st.session_stateonly works when the app is started by Streamlit (i.e., viastreamlit 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 withpython your_script.py, Streamlit emits a warning that session state does not function withoutstreamlit 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 outsidestreamlit 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 likeif '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 withpython 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/settingst.session_stateat 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'] = defaultThis is the documented initialization pattern. [1] Quick diagnosis checklist - Are you launching the app withstreamlit 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 withoutstreamlit run”? That points directly to an inactive/no-runtime session. [2][6][3] - If you are definitely usingstreamlit 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:
streamlit runstreamlit/streamlit#4543Guard
get_bool_param_pairs()for non-Streamlit contexts.The method calls
st.session_state.get()which raisesRuntimeErrorwhen 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
🤖 Prompt for AI Agents