feat: enhanced the visualization of plot theme & application theme #172
feat: enhanced the visualization of plot theme & application theme #172achalbajpai wants to merge 24 commits intoOpenMS:mainfrom
Conversation
WalkthroughThis update adjusts the theming and configuration settings across the project. The configuration file now defines specific theme properties and a new browser setting to disable usage statistics. The common module has been enhanced with new functionality for optional plotting library integration, dynamic theme configuration, and improved error handling for missing dependencies and user interface updates. Additionally, the environment file has been reformatted for clarity. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.streamlit/config.toml (1)
23-24: Font Configuration Left as Default
The font property remains commented out. This is acceptable for now if the default font settings are desired. In future iterations, you might consider making the font configurable for enhanced customization.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.streamlit/config.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-full-app
- GitHub Check: build-openms
- GitHub Check: build-simple-app
- GitHub Check: build
🔇 Additional comments (5)
.streamlit/config.toml (5)
8-9: Enforce Light Mode Theme
The configuration now explicitly forces a light mode theme by settingbase = "light"along with a clarifying comment. This change directly addresses the PR objective of preventing system default theme settings.
15-15: Explicit Background Color Configuration
The main content area's background color is set to white (#FFFFFF), ensuring visual consistency in light mode.
18-18: Defined Secondary Background Color
The sidebar and interactive widget background color is now explicitly defined as light gray (#F0F2F6), which helps maintain a coherent light theme throughout the application.
21-21: Explicit Text Color for Readability
Setting the text color to a dark shade (#262730) ensures sufficient contrast against light backgrounds, thereby improving readability.
26-27: Disabling Browser Usage Statistics
Introducing the[browser]section withgatherUsageStats = falseis a prudent step toward enhancing user privacy by preventing unwanted usage data collection. This change directly aligns with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/common/common.py (1)
264-264: Improve code quality with static analysis suggestionsThere are two minor code quality improvements that could be made to this line.
- if "controllo" not in st.session_state or ( - "controllo" in params.keys() and params["controllo"] == False - ): + if "controllo" not in st.session_state or ( + "controllo" in params and not params["controllo"] + ):🧰 Tools
🪛 Ruff (0.8.2)
264-264: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
264-264: Avoid equality comparisons to
False; useif not params["controllo"]:for false checksReplace with
not params["controllo"](E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/common.py(14 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
264-264: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
264-264: Avoid equality comparisons to False; use if not params["controllo"]: for false checks
Replace with not params["controllo"]
(E712)
🔇 Additional comments (5)
src/common/common.py (5)
10-13: LGTM: Necessary imports added for enhanced plotting capabilitiesThe added imports for plotly, matplotlib, and bokeh are appropriate and necessary for the new theme configuration functionality.
344-365: Well-implemented theme settings in sidebarThe addition of theme settings in the sidebar is well-structured and provides users with clear options to select their preferred plot theme. Good implementation of session state to persist user preferences.
502-639: Comprehensive implementation of plot theme configurationThe
configure_plot_themefunction effectively handles theme configuration for multiple plotting libraries (matplotlib, plotly, and bokeh). The implementation is thorough with specific settings for both light and dark modes, ensuring consistent visualization across the application.
660-723: Theme configuration properly applied to Plotly figuresThe updates to the
show_figfunction correctly apply the theme settings to Plotly figures, ensuring consistent visualization appearance. The conditional logic properly handles different theme modes.
147-150: LGTM: Appropriate tracking consent checkThe tracking consent logic is correctly implemented, ensuring that user preferences are respected and analytics are only enabled when appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
environment.yml (4)
5-5: New Channel Addition:bioconda
The bioconda channel has been added alongside conda-forge. Please ensure the channel priority is as intended and consider adding a brief comment on why the bioconda channel is required for this environment.
12-12: Version Pinning forbokehDependency
Thebokehdependency has been added without an explicit version specifier. For reproducibility, it's recommended to pin the version (e.g.,bokeh==2.4.3) and document the version choice if specific features or stability are required.- - bokeh + - bokeh==<version_number> # replace <version_number> with the desired version
19-19: Confirm Compatibility forpyopenms==3.2.0
Thepyopenms==3.2.0dependency is now added through pip. Please verify that this version is compatible with the specifiednumpy==1.26.4(as noted in the comment) and other dependencies in the environment. Consider adding a note if this specific version is required due to compatibility reasons.
22-22: Assess Version Choice forpsutil==7.0.0
The dependencypsutil==7.0.0is explicitly pinned. Sincepsutilhas seen several updates, please double-check that this version is intended and note any constraints leading to this specific pin. If flexibility is acceptable, version wildcards or a higher version might be considered.- - psutil==7.0.0 + - psutil==<version_number> # update if a newer version is acceptable or document why v7.0.0 is used
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/common/common.py (2)
502-639: Comprehensive theming implementation across multiple visualization libraries.The
configure_plot_themefunction effectively handles theme configuration for matplotlib, plotly, and bokeh. The color selections provide good contrast in both themes and the implementation follows best practices for each library.One suggestion would be to consider extracting the theme configurations to a separate module if this functionality grows in the future, as this function is quite long.
264-265: Simplify condition check for better readability.The current condition uses
.keys()unnecessarily and directly compares a boolean toFalse.-if "controllo" not in st.session_state or ( - "controllo" in params.keys() and params["controllo"] == False -): +if "controllo" not in st.session_state or ( + "controllo" in params and not params["controllo"] +):🧰 Tools
🪛 Ruff (0.8.2)
264-264: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
264-264: Avoid equality comparisons to
False; useif not params["controllo"]:for false checksReplace with
not params["controllo"](E712)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
environment.yml(1 hunks)src/common/common.py(14 hunks)test_gui.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
264-264: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
264-264: Avoid equality comparisons to False; use if not params["controllo"]: for false checks
Replace with not params["controllo"]
(E712)
🔇 Additional comments (12)
src/common/common.py (3)
10-13: Good addition of necessary imports for theme configuration.These imports are required for the new theming functionality and are appropriately placed at the top of the file.
344-356: Well-implemented theme selection UI.The theme settings interface is cleanly integrated into the sidebar with appropriate default values and help text. The theme options ("system", "light", "dark") provide good flexibility for users.
660-723: Good integration of theme configuration with the existing plotting functionality.The updates to the
show_figfunction properly apply the selected theme to plotly charts. The conditional styling based on the theme ensures consistency across the application.environment.yml (3)
5-5: Addition of bioconda channel is appropriate.Adding the bioconda channel is a good choice for accessing bioinformatics packages.
11-14: Good addition of dependencies with specific versions.The added dependencies (bokeh, pandas, pytest) with pinned versions ensure compatibility and reproducibility. The comment explaining the addition of pandas and pytest is helpful for future maintainers.
20-23: Appropriate placement of pip-specific dependencies.Moving pyopenms to the pip section and adding psutil with a specific version is appropriate. These dependencies align with the updated functionality in the codebase.
test_gui.py (6)
16-17: Consistent string formatting in test configuration.The change to use double quotes instead of single quotes improves consistency with the rest of the codebase.
22-39: Improved test parameterization formatting.The reformatting of test parameters into a multi-line tuple improves readability without changing functionality.
42-42: Increased timeout improves test reliability.Increasing the timeout from 3 to 10 seconds will make the tests more reliable, especially in CI/CD environments where resources might be constrained.
46-60: Consistent formatting for parameterized tests.The restructured parameter formatting makes the test definitions more readable and maintainable.
77-86: Well-formatted test parameterization.The parameterization is now more readable with one test case per line in a clear structure.
103-113: Consistent test parameterization style.This formatting change maintains the same improved style as the other parameterized tests in the file.
achalbajpai
force-pushed
the
new-testing
branch
from
0af3423 to
b1b9e6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
environment.yml(1 hunks)src/common/common.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
- GitHub Check: build-openms
🔇 Additional comments (8)
src/common/common.py (4)
10-13: Added necessary imports for theme configurationThe new imports for Plotly, Matplotlib, and Bokeh support the theme configuration functionality that's being implemented. These are appropriate libraries for the task of customizing plot appearances.
377-389: Good addition of theme customization optionsThe new theme settings section in the sidebar provides users with the ability to choose between system, light, and dark themes. This aligns with the PR objective to control theming, while still offering flexibility for users who may prefer different themes.
538-676: Well-implemented theme configuration functionThe
configure_plot_themefunction is comprehensive and handles theme configuration for multiple visualization libraries (Matplotlib, Plotly, and Bokeh). The function properly handles both light and dark themes with detailed color settings that ensure good contrast and readability.The implementation is robust, with appropriate color choices for each theme and careful attention to accessibility considerations.
696-759: Effective integration of theme configuration in the show_fig functionThe updates to the
show_figfunction properly integrate the new theming capabilities by callingconfigure_plot_theme()and updating the Plotly figure layout based on the selected theme. The explicit settings for background colors, grid colors, and font colors ensure a consistent appearance across the application.environment.yml (4)
5-5: Added bioconda channel for expanded package accessAdding the bioconda channel is appropriate as it provides access to bioinformatics packages that may be relevant for this application.
11-12: Added explicit dependencies with version specificationsGood practice to specify exact versions for pandas and pytest. This ensures reproducibility and consistent behavior across different environments.
21-21: Added pyopenms dependencyAdding the pyopenms package with a specific version is appropriate and follows the version specification pattern used for other dependencies.
24-25: Moved bokeh to pip dependencies with version specificationMoving bokeh to pip dependencies with a specific version is good for cross-platform compatibility as mentioned in the comment. However, as noted earlier, bokeh is also listed as a conda dependency which could cause conflicts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test_gui.py (1)
99-99: Consider addressing the TODO comment.There's an unresolved TODO comment about selecting a spectrum to be displayed. Consider implementing this functionality or creating a separate issue to track it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/common/common.py(4 hunks)test_gui.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-full-app
- GitHub Check: build-openms
- GitHub Check: build-simple-app
🔇 Additional comments (8)
src/common/common.py (3)
10-13: Good addition of necessary imports.The new imports for plotly, matplotlib, and bokeh are necessary for the theme configuration functionality. These libraries are correctly imported with appropriate aliases.
377-389: Well-structured theme settings implementation.The addition of theme settings in the sidebar provides users with good customization options. The implementation is clean with proper default value handling and helpful tooltips.
696-759: Good integration of theme configuration with figure display.The updates to the
show_figfunction properly integrate the theme configuration before displaying figures and apply consistent styling to Plotly figures. The implementation handles both light and dark themes with appropriate color schemes and styling parameters.test_gui.py (5)
16-17: Proper test configuration.Setting
testto True in settings and adding the workspace secret is necessary for proper test execution.
22-39: Improved parameterization readability.The multi-line formatting of the parameterization decorator enhances code readability, especially for longer parameter lists.
42-42: Appropriate timeout increase.Increasing the timeout from 3 to 10 seconds provides more time for the application to launch, which is reasonable for complex UI initialization, especially with the new theming functionality.
47-61: Enhanced readability with proper formatting.The multi-line formatting of the test parameters improves code readability and maintainability.
104-114: Improved test parameter formatting.The updated formatting enhances readability of the test parameters.
Thanks for updating the PR. Looks like we found a good solution to display dark mode for the plots. In this case of course light mode should not be fixed. Could you update the toggle to change the global theme as you suggested initially? I also have some thoughts regarding the existing code so please also consider my review! |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
environment.yml(1 hunks)requirements.txt(1 hunks)src/common/common.py(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-simple-app
- GitHub Check: build-full-app
- GitHub Check: build-openms
🔇 Additional comments (10)
src/common/common.py (6)
11-18: Good approach for optional Plotly imports.
By wrapping Plotly imports in atry/except, the code gracefully handles the scenario where Plotly is not installed.
19-25: Consistent optional import pattern for Matplotlib.
Similarly, the logic forMPL_AVAILABLEfollows the same pattern as Plotly, which keeps the code robust when libraries are missing.
26-33: Optional Bokeh import is well-structured.
Your continuing pattern of guarded imports (for Bokeh) is consistent and helps prevent runtime errors.
36-42: Optional psutil import.
This maintains consistent handling of optional dependencies. Good job ensuring the rest of the module can function withoutpsutil.
59-61: Clear warning for missing psutil.
Displaying a warning is a user-friendly approach to inform about unavailable resource monitoring.
728-791: Plotly theme logic duplicates dark mode support.
In line with enforcing a global light theme, retaining the dark color scheme might be unnecessary. Double-check whether these two paths are truly needed.requirements.txt (1)
11-12: Added testing & data manipulation libraries.
Includingpandas==2.2.1andpytest==8.0.0is beneficial. Please verify compatibility with existing code and ensure no version conflicts with your environment constraints.environment.yml (3)
5-5: New channel: bioconda.
This channel can be important for bioinformatics packages. Validate that it does not conflict with conda-forge dependencies.
11-12: Consistency in adding pandas & pytest.
Aligns with the changes made in requirements.txt. Ensure the pinned versions match all distribution environments.
19-19: Introduced pyopenms in pip dependencies.
Double-check for version compatibility with conda installation, especially since pyopenms also depends on numpy.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/common/common.py (1)
743-805: 🛠️ Refactor suggestionWell-implemented but check for library availability before configuration.
The
configure_plot_themefunction is well structured with comprehensive styling for multiple visualization libraries. However, you should check for library availability at the beginning of the function to prevent any runtime errors.def configure_plot_theme(): """Configure plot themes based on Streamlit's theme.""" # Get the current theme based on user preference theme_mode = st.session_state.plot_theme - if MPL_AVAILABLE: + # Early return if no plotting libraries are available + if not any([MPL_AVAILABLE, PLOTLY_AVAILABLE, BOKEH_AVAILABLE]): + return + + if MPL_AVAILABLE:
🧹 Nitpick comments (2)
src/common/common.py (2)
507-571: File modification concerns in certain environments.Directly modifying the
.streamlit/config.tomlfile might cause issues in certain deployment environments where file writes are restricted (like containerized deployments or read-only filesystems). Consider:
- Using Streamlit's session state for temporary theme changes
- Implementing theme changes through environment variables
- Adding checks for file write permissions before attempting modifications
573-593: Potential UX inconsistency with separate theme controls.Having separate controls for application theme and plot theme could lead to a confusing user experience if they don't match. Consider:
- Syncing the plot theme with the application theme automatically
- Adding a note explaining that plot themes can be different from the application theme
- Providing visual feedback showing how plots will appear with the selected theme
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/common.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-openms
- GitHub Check: build-full-app
- GitHub Check: build-simple-app
🔇 Additional comments (5)
src/common/common.py (5)
11-32: Good implementation of optional plotting package imports.The conditional imports for plotting libraries (plotly, matplotlib, bokeh) with availability checks is a good practice. This makes these dependencies optional rather than required, which improves the application's flexibility.
37-42: Well-handled psutil optional dependency.Making psutil optional with appropriate availability checks is a good approach, especially if this dependency has been removed from environment files as mentioned in the AI summary.
57-66: Effective caching implementation for system metrics.Using
st.cache_resourcewith a TTL parameter is an excellent optimization to prevent frequent updates of system metrics, reducing unnecessary system calls.
68-82: Good enhancement to resource monitoring function.The modified
monitor_hardwarefunction now properly handles the case where psutil is not available and uses the cached metrics function. This makes the code more robust and maintainable.
825-881: Good plot rendering with theme-aware configuration.The modifications to
show_figeffectively apply theme configuration before displaying figures and update the Plotly figure layout based on the selected theme. This ensures visual consistency.
achalbajpai
force-pushed
the
new-testing
branch
from
98a81d9 to
65a79bf
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.streamlit/config.toml (1)
22-23: Optional: Font Customization ReminderThe
fontproperty remains commented out. If a consistent typography is desired across the application, please consider specifying a default font family.
🛑 Comments failed to post (2)
.streamlit/config.toml (2)
13-19: 🛠️ Refactor suggestion
⚠️ Potential issueColor Configuration Mismatch
According to the PR objectives, the color values should support a light mode theme:
- The main content background should be white (
#FFFFFF) instead of the current dark color (#0E1117).- The sidebar background should use a light gray (e.g.,
#F0F2F6) rather than#262730.- The text color should be a dark shade (
#262730) for good contrast on a light background, instead of#FAFAFA.Please update these values accordingly for a consistent light mode theme.
- backgroundColor = "#0E1117" + backgroundColor = "#FFFFFF" - secondaryBackgroundColor = "#262730" + secondaryBackgroundColor = "#F0F2F6" - textColor = "#FAFAFA" + textColor = "#262730"📝 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.backgroundColor = "#FFFFFF" # Background color used for the sidebar and most interactive widgets. secondaryBackgroundColor = "#F0F2F6" # Color used for almost all text. textColor = "#262730"
8-8: 🛠️ Refactor suggestion
⚠️ Potential issueTheme Mode Mismatch
The PR objective states that the application should enforce a light mode theme, but here the theme is set to dark mode with
base = "dark". Please update this value to"light"to align with the intended behavior.- base = "dark" + base = "light"📝 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.base = "light"
achalbajpai
force-pushed
the
new-testing
branch
2 times, most recently
from
32face0 to
a3253e4
Compare
…deprecation warning
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/common/common.py (3)
401-561:⚠️ Potential issueTheme toggle conflicts with PR objective “enforce light mode.”
The code introduces a user-selectable theme (“light” or “dark”), yet the PR objective explicitly states a requirement to prevent system defaults and enforce a light theme. This implementation allows users to switch to dark mode, contradicting that objective.To align with the PR goal of a strictly enforced light mode, consider removing the dark theme path and user toggle. You can still provide customizeable light-mode variations if brand guidelines require it, but dark mode is out of scope per the stated objective.
Below is an example diff that removes the theme toggle in this block and defaults to the light theme only:
- # Application Theme settings - st.markdown("## Application Theme") - theme_options = ["light", "dark"] - ... - # Entire logic for reading config.toml and switching to "dark" - # or rewriting colors for "dark" can be removed. + st.markdown("**Application theme is enforced as light**") + # If desired, you can still override specific light colors here, + # but no toggling to dark mode.
732-794:⚠️ Potential issueDark-mode branch in
configure_plot_themeconflicts with enforced light mode.
This function diligently configures theme colors for matplotlib, Plotly, and Bokeh in both light and dark modes. In light of the PR objective, consider removing the dark-mode logic and focusing strictly on a single light-mode configuration.def configure_plot_theme(): """Configure plot themes based on Streamlit's theme.""" - theme_mode = st.session_state.plot_theme - if MPL_AVAILABLE: - if theme_mode == "light": - plt.style.use("default") - else: - plt.style.use("dark_background") + # Force a single light style + if MPL_AVAILABLE: + plt.style.use("default") - if PLOTLY_AVAILABLE: - if theme_mode == "light": - pio.templates.default = "seaborn" - else: - pio.templates.default = "plotly_dark" + if PLOTLY_AVAILABLE: + pio.templates.default = "seaborn" - if BOKEH_AVAILABLE: - if theme_mode == "light": - # Light theme configuration - ... - else: - # Dark theme configuration - ... + if BOKEH_AVAILABLE: + # Apply a single light-theme JSON + ...
562-583:⚠️ Potential issuePlot theme toggle also contradicts enforced light mode.
Similar to the application theme toggle, the “Plot Theme” selectbox here reintroduces a dark option. If you truly must enforce light mode globally, remove or disable the dark path for plots.- st.markdown("## Plot Theme") - ... - theme_options = ["light", "dark"] - st.selectbox( - "Plot Theme", - theme_options, - index=theme_options.index(st.session_state.plot_theme), - key="plot_theme", - ... - ) + st.markdown("**Plot Theme is enforced: light**") + # Remove or disable the dark mode option
🛑 Comments failed to post (1)
src/common/common.py (1)
814-870:
⚠️ Potential issueDark layout logic in
show_figfunction also undermines light-only enforcement.
This block updates Plotly chart layouts for both “light” and “dark.” Remove the dark variants if you intend a purely light environment.if hasattr(fig, "update_layout"): - if theme_mode == "light": - fig.update_layout( - paper_bgcolor="white", - ... - ) - else: - fig.update_layout( - paper_bgcolor="#0E1117", - ... - ) + fig.update_layout( + paper_bgcolor="white", + plot_bgcolor="white", + font_color="black", + ... + )📝 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.# Configure plot theme before displaying configure_plot_theme() # Get current theme based on plot theme setting theme_mode = st.session_state.plot_theme # Update Plotly figure layout based on theme if hasattr(fig, "update_layout"): - if theme_mode == "light": - fig.update_layout( - paper_bgcolor="white", - plot_bgcolor="white", - font_color="black", - xaxis=dict( - gridcolor="lightgray", - gridwidth=1, - griddash="dash", - linecolor="black", - linewidth=1, - ticks="outside", - tickfont=dict(color="black"), - ), - yaxis=dict( - gridcolor="lightgray", - gridwidth=1, - griddash="dash", - linecolor="black", - linewidth=1, - ticks="outside", - tickfont=dict(color="black"), - ), - ) - else: - fig.update_layout( - paper_bgcolor="#0E1117", - plot_bgcolor="#0E1117", - font_color="#FFFFFF", # Brighter white for better contrast - xaxis=dict( - gridcolor="#555555", # Slightly brighter grid - gridwidth=1, - griddash="dash", - linecolor="#FFFFFF", # Brighter white for better contrast - linewidth=1, - ticks="outside", - tickfont=dict(color="#FFFFFF"), - ), - yaxis=dict( - gridcolor="#555555", # Slightly brighter grid - gridwidth=1, - griddash="dash", - linecolor="#FFFFFF", # Brighter white for better contrast - linewidth=1, - ticks="outside", - tickfont=dict(color="#FFFFFF"), - ), - ) + fig.update_layout( + paper_bgcolor="white", + plot_bgcolor="white", + font_color="black", + xaxis=dict( + gridcolor="lightgray", + gridwidth=1, + griddash="dash", + linecolor="black", + linewidth=1, + ticks="outside", + tickfont=dict(color="black"), + ), + yaxis=dict( + gridcolor="lightgray", + gridwidth=1, + griddash="dash", + linecolor="black", + linewidth=1, + ticks="outside", + tickfont=dict(color="black"), + ), + )
achalbajpai
force-pushed
the
new-testing
branch
2 times, most recently
from
0b009f4 to
ca8e8fd
Compare
…ed theme system, and cleaned up code
achalbajpai
force-pushed
the
new-testing
branch
from
218a560 to
3cece29
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/common/common.py (1)
452-609:⚠️ Potential issueConflict with PR objective to enforce light mode.
While this code allows the user to switch to "system" or "dark" themes, the PR objective is to enforce a consistent light theme and avoid system overrides. If strict enforcement is desired, remove or disable these other options to ensure the application remains in light mode.
A minimal fix would be to restrict the theme options to only light mode, for example:
- theme_options = ["system", "light", "dark"] + theme_options = ["light"]You might also remove or comment out the code blocks that apply the dark theme CSS and update
config.tomlfor dark mode to prevent any user override.
🧹 Nitpick comments (1)
src/common/common.py (1)
26-32: UnusedThemeimport detected.According to static analysis,
bokeh.themes.Themeis not actually used in the code. Consider removing it to keep your imports clean.- from bokeh.themes import Theme from bokeh.io import curdoc🧰 Tools
🪛 Ruff (0.8.2)
27-27:
bokeh.themes.Themeimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.streamlit/config.toml(1 hunks)environment.yml(1 hunks)src/common/common.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- environment.yml
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
27-27: bokeh.themes.Theme imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (16)
.streamlit/config.toml (2)
10-22: Great job enforcing the light mode theme.The
baseproperty is correctly set to"light", and the background/color settings align well with the PR objective to enforce a consistent light mode.
27-28: Explicitly disabling usage stats.This change prevents Streamlit from collecting usage statistics. If this aligns with your privacy or organizational requirements, it looks good.
src/common/common.py (14)
11-18: Optional Plotly import handled gracefully.Good approach using a try/except block to conditionally set
PLOTLY_AVAILABLE. This avoids breaking functionality if Plotly is not installed.
19-24: Optional Matplotlib import handled properly.Similar to Plotly, this is a good pattern for gracefully handling missing optional dependencies.
36-43: Optional Pandas import seems well-structured.This approach correctly falls back to a substitute class if Pandas is not available, preventing runtime errors.
44-63: Minimal DataFrame substitute is a practical solution.Providing a lightweight fallback class avoids breaking the UI in environments lacking Pandas.
65-70: Optional psutil import is solid.Good pattern for systems where
psutilis not installed. The rest of the code remains functional without it.
87-118: Robust error handling inmonitor_hardware.Catching errors and providing warnings is beneficial for different platforms and configurations. Nicely done.
211-211: Added page configuration comment.This clarifies the placement of
st.set_page_config. No issues found.
611-619: Export settings block looks fine.Providing a selectbox for image export formats is straightforward and matches the rest of your UI design.
692-695: Graceful fallback for missing Pandas indisplay_large_dataframe.Showing a warning and proceeding is user-friendly and consistent with your optional dependency approach.
717-741: Chunk-based DataFrame display logic is clear.Paginating large data in manageable chunks helps performance and user experience. Good implementation.
759-779:show_tablefallback path is well-handled.Continuing to display minimal table info when Pandas is absent avoids breaking the UI.
781-815: Dark theme handling duplicates the same conflict.This function configures both light and dark themes, conflicting with the PR objective of strictly enforcing light mode (already flagged in lines 452-609).
835-944: Plot layout logic again provides dark mode support.As with earlier comments, this duplicates the conflict with your strict light mode requirement.
976-986: Memory usage calculation is well-handled.Returning zero when Pandas is unavailable prevents potential crashes and keeps the flow consistent.
|
Let me know if the PR is ready for review. |
|
Hi @t0mdavid-m , yes it is , have upload the final video sample in #168 (comment) |
t0mdavid-m
left a comment
t0mdavid-m
left a comment
There was a problem hiding this comment.
Looks good overall! Thanks for the changes. I think there was a bit of miscommunication with checking if packages are available. We only need to do this for the plotting packages as they are optional dependencies (i.e. the user can decide which one they want to use and then only install the required ones). All other packages are not optional dependencies and we can thus assume they are installed.
Could you also doublecheck whether the modification of the css styling is neccessary (see relevant comment)?
| # Apply immediate visual feedback for the current session | ||
| if selected_theme == "dark": | ||
| # Apply dark theme CSS immediately | ||
| st.markdown( |
There was a problem hiding this comment.
Is this really necessary? I am a bit fearful that this could be a lot of effort to maintain if Streamlit changes the way they do this internally (i.e. it relies on implementation details). Did you have trouble updating the config in a running server?
There was a problem hiding this comment.
Is this really necessary? I am a bit fearful that this could be a lot of effort to maintain if Streamlit changes the way they do this internally (i.e. it relies on implementation details). Did you have trouble updating the config in a running server?
Yes I did have problem updating the config in a running server ,
After extensive testing and research, I've discovered this is a fundamental limitation with Streamlit's theming system.
I tried around 3 approaches , the current one fits the best ( session state , css + selector didnt work properly at all cases )
The Streamlit documentation acknowledges there's no officially supported way to programmatically change themes at runtime , other than the one I tried to figure out.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/common/common.py (3)
15-36: Consider refactoring repeated optional import logic
The try-except blocks for optional libraries (plotly,matplotlib, andbokeh) look fine. As a minor improvement, consider factoring out this pattern into a small helper function to reduce repetition.
416-553: Refactor dynamic config writes for maintainability
Rewriting the.streamlit/config.tomlon the fly can complicate concurrency or multi-user scenarios (e.g., conflicting writes). Consider extracting this logic into a dedicated function that locks or queues writes, ensuring more control and clarity.
662-686: Remove leftover debug statement
Theprint(base_index)call at line 684 appears to be a debug artifact. Consider removing or replacing it with proper logging to keep production code clean.- print(base_index)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/common.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/common/common.py
637-637: Undefined name PANDAS_AVAILABLE
(F821)
704-704: Undefined name PANDAS_AVAILABLE
(F821)
921-921: Undefined name PANDAS_AVAILABLE
(F821)
🔇 Additional comments (8)
src/common/common.py (8)
12-13: No issues with added import statement
No concerns here; the additional import ofstreamlitis straightforward.
53-53: Docstring change looks good
The updated docstring clearly states the function's purpose.
156-156: No concerns regarding page configuration comment
The explanatory comment is fine and aligns with Streamlit requirements.
397-414: Verify “system” theme detection logic
When “system” is selected, you rely onst.get_option("theme.base")to indicate “light” or “dark.” Confirm that no mismatch occurs if the config is actually set to “system.” You may want to handle a true "system" mode or fallback more explicitly.
687-689: Error handling block looks fine
Showing a warning and displaying fallback content on exception is a good approach here.
709-724: Data table display and download flow
The fallback if pandas is unavailable is handled nicely, and the try-except pattern is appropriate for error resilience.
726-760: Plot theme configuration is clear
This function successfully covers all three libraries with safe try-except usage and straightforward style application.
762-893:show_figimplementation seems solid
Passing the figure through theme updates and customizing layout based on the selected theme is a comprehensive approach. No major concerns.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/common/common.py (3)
15-21: Consolidate optional imports for maintainability.
The repeatedtry/exceptblocks for Plotly, Matplotlib, and Bokeh are fine for handling optional dependencies. However, you could consider factoring them into a common helper to reduce repetition and streamline installation checks.Also applies to: 23-28, 30-35
703-737: Central theme configuration is well-structured.
Theconfigure_plot_themefunction cleanly configures Matplotlib, Plotly, and Bokeh themes. If you plan to add more libraries in the future, consider exposing a single uniform interface or hooking into session state to avoid repeated calls.
763-867: Dynamic layout updates are good, but consider DRY.
The code to update layout for light/dark themes is thorough but repetitive. Refactoring into a helper function or dictionary-driven approach could reduce duplication and improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/common.py(6 hunks)
🔇 Additional comments (6)
src/common/common.py (6)
12-13: Use caution with library version management.
The addition ofimport streamlit as stis correct, but ensure that the Streamlit version in use supports all utilized features.
53-54: Docstring update confirmed.
The docstring formonitor_hardwareis concise and clear.
156-157: Ensure page config is set before other Streamlit calls.
Callingst.set_page_configfirst is a best practice to avoid exceptions. This looks good.
555-556: Export and spectrum plotting configuration look good.
The additions for image export and spectrum plotting are straightforward. Be sure to add or update tests if needed.Also applies to: 564-565
757-760: Graceful handling of missing Plotly.
Showing a warning and halting execution of the function is a reasonable approach. This avoids runtime errors if Plotly is absent.
868-869: Robust error handling for figure display.
Catching exceptions and showing a user-facing warning helps avoid silent failures. Consider logging errors as well if you have a logging setup.
achalbajpai
force-pushed
the
new-testing
branch
2 times, most recently
from
a1ff67c to
ee79da8
Compare
Hi @t0mdavid-m ,
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
environment.yml (1)
38-54: Remaining Pip Dependencies: Documentation and Essentiality
The final block lists additional pinned packages (frompyparsingtowatchdog). While pinning versions ensures reproducibility, please review each dependency to confirm that it is essential for supporting the application's enhanced theming and visualization features. Documenting any non-obvious dependencies could also aid future maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
environment.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-openms
- GitHub Check: build-full-app
🔇 Additional comments (6)
environment.yml (6)
3-3: Python Version Update: Verify Intentional Downgrade
The Python version is now set to 3.10. Please confirm that this change (if previously using a later version such as 3.11) is deliberate and that all dependencies and project features are compatible with Python 3.10.
4-5: Pip Dependency Declaration: Structure Check
The inclusion of both- pipand the nested- pip:block for specifying pip packages is a conventional practice in conda environments. Confirm that this structure meets your project’s dependency management expectations.
6-9: UI and Plotting Dependencies: Version Compatibility
The added pip dependencies starting withaltair==5.5.0,attrs==25.3.0,blinker==1.9.0, andcachetools==5.5.2are likely tied to UI enhancements and visualization features. Please verify that their pinned versions are fully compatible with the overall theming and plotting improvements introduced in this PR.
10-10: UnexpectedcaptchaDependency: Confirmation Needed
The addition ofcaptcha==0.5.0is not clearly aligned with the PR’s focus on enhancing plot and application themes. Please confirm whether this dependency is required for new functionality, or if it has been included inadvertently.
11-35: Pinned Dependencies for Core Libraries: Compatibility & Relevance
This block pins several important libraries (e.g.,certifi,charset-normalizer,click,contourpy,cycler,matplotlib==3.10.1,narwhals,numpy,pandas,plotly==5.22.0, etc.). Ensure that these versions are chosen to support the enhanced theming functionalities without causing conflicts. It might also be helpful to review if any of these packages could be updated or streamlined in future iterations for maintainability.
36-37: PyOpenMS and PyOpenMS-Viz Dependencies: Revisit Necessity
Bothpyopenms==3.2.0andpyopenms-viz==1.0.0are included. As noted in previous reviews, these dependencies might not be needed unless they serve a specific purpose in your Docker build or overall application functionality. Please confirm their necessity, and if they are not required, consider removing them to avoid unnecessary bloat.
2 participants
Solves #168
Changes Made
Theme Implementation Details
Demo
Screen.Recording.2025-03-20.at.8.23.49.PM.mov
Testing Done
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements