-
Notifications
You must be signed in to change notification settings - Fork 35
Add convenience function for retrieving parameters #301
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
Conversation
📝 WalkthroughWalkthroughAdded a new public method to ParameterManager that retrieves merged TOPP tool parameters by loading defaults from an ini file, extracting tool-specific parameters, and overlaying user-provided JSON values to produce a consolidated parameter dictionary. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
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.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/workflow/ParameterManager.py
🔇 Additional comments (2)
src/workflow/ParameterManager.py (2)
101-114: LGTM! Clear method signature with defensive check.The method signature, docstring, and defensive check for ini file existence are well-implemented. The documentation clearly explains the purpose and return behavior.
120-133: LGTM! Merge logic correctly implemented.The implementation correctly:
- Extracts parameters with the tool-specific prefix
- Handles both bytes and string keys from the pyopenms API
- Merges user overrides on top of defaults using
update()The logic is consistent with the existing parameter handling patterns in
save_parameters().
| # Load defaults from ini file | ||
| param = poms.Param() | ||
| poms.ParamXMLFile().load(str(ini_path), param) |
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.
Add error handling for corrupted ini files.
The ParamXMLFile().load() call lacks error handling for corrupted or malformed ini files, which could raise an unhandled exception and disrupt the application. Compare this with get_parameters_from_json() (lines 94-99), which includes a try-except block to handle invalid JSON files gracefully.
🔎 Proposed fix with error handling
# Load defaults from ini file
param = poms.Param()
- poms.ParamXMLFile().load(str(ini_path), param)
+ try:
+ poms.ParamXMLFile().load(str(ini_path), param)
+ except Exception:
+ st.error(f"**ERROR**: Failed to load ini file for tool '{tool}'. File may be corrupted.")
+ return {}🤖 Prompt for AI Agents
In src/workflow/ParameterManager.py around lines 116-118, the
poms.ParamXMLFile().load(...) call lacks error handling for malformed/corrupted
ini files; wrap that load call in a try/except block (matching the pattern used
in get_parameters_from_json at lines 94-99), catch parsing/file exceptions
(catch Exception as e if no specific poms exception is available), log the error
including exception details (using the module logger or logging.exception), and
then fall back to an empty/default poms.Param() or return/continue gracefully so
the application does not crash.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.