Fix the documentation issue #174#191
Conversation
- The documentation mentions openms.ico as the app icon for the Windows executable. - However, the existing openms.ico in the �ssets folder contained the old logo. - Replaced openms.ico with an updated version matching openms_transparent_bg_logo.svg to maintain consistency with the documentation. This ensures the Windows executable uses the latest logo while keeping the .ico format for compatibility
WalkthroughThe changes improve documentation clarity and code readability across several files. The user guide now clearly distinguishes between Online and Local Modes for workspace access. File paths in build documentation have been updated, and workflow documentation has been expanded with refined instructions and a new Workflow member. Additionally, minor code adjustments in the main application file include explicit imports for JSON handling and additional whitespace for readability. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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 (
|
… automatically after changes so deleting it
![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
🧹 Nitpick comments (1)
docs/user_guide.md (1)
27-30: Format and clarity improvements for workspace access section.The changes improve clarity about workspace behavior in different modes and enhance readability with proper formatting.
There's a double punctuation issue in line 30. Consider fixing:
- - The workspace directory can be specified in `settings.json`. - - Defaults to `..` (parent directory). + - The workspace directory can be specified in `settings.json`. + - Defaults to `../` (parent directory).🧰 Tools
🪛 LanguageTool
[typographical] ~30-~30: Two consecutive dots
Context: ...insettings.json. - Defaults to..(parent directory). ## Downloading Re...(DOUBLE_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/openms.icois excluded by!**/*.ico
📒 Files selected for processing (4)
app.py(1 hunks)docs/toppframework.py(1 hunks)docs/user_guide.md(1 hunks)docs/win_exe_with_pyinstaller.md(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app.py
3-3: src.common.common.captcha_control imported but unused
Remove unused import: src.common.common.captcha_control
(F401)
7-7: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
🪛 LanguageTool
docs/user_guide.md
[typographical] ~30-~30: Two consecutive dots
Context: ...in settings.json. - Defaults to .. (parent directory). ## Downloading Re...
(DOUBLE_PUNCTUATION)
🔇 Additional comments (3)
docs/toppframework.py (1)
39-39: Documentation updated to reflect the five important members.The documentation now correctly specifies that the
Workflowclass contains five important members rather than four, which accurately reflects the class implementation.docs/win_exe_with_pyinstaller.md (1)
26-28: Good addition of environment verification step.Adding verification of the Python environment before installing PyInstaller is a useful improvement that will help users ensure they're using the correct environment, preventing common setup issues.
docs/user_guide.md (1)
23-23: Improved clarity regarding online mode file uploads.The documentation now clearly specifies that file uploads in online mode are "Not Enabled in Default," which helps prevent user confusion.
t0mdavid-m
left a comment
t0mdavid-m
left a comment
There was a problem hiding this comment.
Thank you for your PR. However, the issue was about fixing the misaligned file paths in the documentation. Could you adjust those as well?
I just saw you asked questions regarding this on Discord:
HI @tom Müller @arslan Siraj
@josh In the developer documentation, it states that to create a custom workflow, we need to edit src/TOPPWorkflow.py. However, this file does not exist in the src folder. Based on the available files, src/WorkflowManager.py appears to serve the same purpose and can be used instead to define and manage workflows. Am I correct in assuming this?
Almost! src/Workflow needs to be edited, which has an example workflow (all workflows inherit from the class you mentioned though).
the contetn/6_TOPP-Workflow also not exist but the content of doucmenation is present in docs/toppframework.py
This is split across multiple files now:topp_workflow_X.py.
Could you kindly check if other file paths in the documentation are also invalid? Feel free to ask questions here. We go through the PRs and issues regularly, so you will get a response as soon as we have the bandwidth.
|
Could you also merge with the main branch and fix the conflicts? |
|
acknowlege |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/user_guide.md (1)
30-30: Fix typographical error in directory path notation.The text refers to the parent directory as
..which is correct in code but could be confusing in documentation.- - Defaults to `..` (parent directory). + - Defaults to the parent directory (`../`).🧰 Tools
🪛 LanguageTool
[typographical] ~30-~30: Two consecutive dots
Context: ...insettings.json. - Defaults to..(parent directory). ## Downloading Re...(DOUBLE_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app.py(1 hunks)docs/user_guide.md(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
app.py
6-6: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
🪛 LanguageTool
docs/user_guide.md
[typographical] ~30-~30: Two consecutive dots
Context: ...in settings.json. - Defaults to .. (parent directory). ## Downloading Re...
(DOUBLE_PUNCTUATION)
🔇 Additional comments (6)
app.py (3)
3-3: LGTM! Added blank line improves readability.Adding a blank line after imports helps visually separate import statements from the rest of the code, improving readability.
8-8: LGTM! Another blank line for better code organization.This additional blank line creates a clear separation between imports and the application code, which is a good practice for code organization.
6-6: Unused import warning.The
pyopenmsimport is flagged as unused by the static analyzer, but the comment on line 5 explains that it's required for the Windows version to function properly.Consider adding a more explicit comment that explains why this import is needed even though it's not directly used, to avoid future confusion or removal attempts:
-# For some reason the windows version only works if this is imported here +# DO NOT REMOVE: The Windows version only works when pyopenms is imported here, +# even though it's not directly used in this file🧰 Tools
🪛 Ruff (0.8.2)
6-6:
pyopenmsimported but unusedRemove unused import:
pyopenms(F401)
docs/user_guide.md (3)
21-21: LGTM! Improved documentation structure.Adding an empty line after the heading helps with readability and follows proper markdown formatting practices.
25-25: LGTM! Added line break improves readability.The additional blank line creates a clear separation between different sections in the documentation.
27-30: Improved workspace access documentation.The changes clarify the workspace behavior in online and local modes, providing users with important information about data persistence and configuration options.
🧰 Tools
🪛 LanguageTool
[typographical] ~30-~30: Two consecutive dots
Context: ...insettings.json. - Defaults to..(parent directory). ## Downloading Re...(DOUBLE_PUNCTUATION)
done |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/user_guide.md (1)
39-42: Enhance Clarity on Workspace Persistence Mode.
The updated "Workspace Access" section now clearly differentiates between Online Mode (temporary storage with a seven-day inactivity clearance) and Local Mode (persistent storage on the local machine). Specifying that the workspace directory can be configured viasettings.jsonand defaults to..(parent directory) is useful for clarity.Suggestion: Consider adding a brief explanatory note about why
..(parent directory) was chosen as the default. This additional context could help beginners better understand the underlying file structure and rationale behind the configuration.🧰 Tools
🪛 LanguageTool
[typographical] ~42-~42: Two consecutive dots
Context: ...insettings.json. - Defaults to..(parent directory). ## Downloading Re...(DOUBLE_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/user_guide.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/user_guide.md
[typographical] ~42-~42: Two consecutive dots
Context: ...in settings.json. - Defaults to .. (parent directory). ## Downloading Re...
(DOUBLE_PUNCTUATION)
|
|
|
Hi @t0mdavid-m , I noticed that the developer documentation mentions: “All input files for the workflow will be stored within the workflow directory in the subdirectory input-files...” However, based on the current implementation, input files are actually stored inside the active workspace directory (i.e., st.session_state.workspace) under a subfolder named by the upload key (e.g., mzML-files). There’s no input-files directory in the repo, and this could be misleading for new contributors. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/build_app.md (1)
33-42: Documentation File Path Update: Correct File ReferenceThe documentation now correctly directs users to update the app name and repository name in
src/common/common.pyinstead ofsrc/common.py. This change improves clarity regarding the actual file structure of the project. Please verify that all related documentation references align with the updated directory hierarchy, and consider adding a brief note explaining this change for the benefit of new contributors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/OpenMS.pngis excluded by!**/*.png
📒 Files selected for processing (2)
docs/build_app.md(1 hunks)docs/toppframework.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/toppframework.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.streamlit/config.toml (2)
7-11: Clarifications in the Theme Section
The[theme]section now features clearer inline comments (e.g., explaining the inherited preset theme). One suggestion: if many users are expected to customize the base theme, consider either enabling a default value forbaseor including an explanatory note as to why it remains commented out.
24-25: Preservation of Font Option Documentation
The commented-outfontsetting is retained, which is fine if you rely on the default font configuration. However, for contributors who may wish to customize typography, consider adding a brief note in the README regarding how to enable or override this setting.docs/user_guide.md (1)
27-27: Fix the double punctuation.There are two consecutive dots in this line which appears to be a typographical error.
- - The workspace directory can be specified in `settings.json` . + - The workspace directory can be specified in `settings.json`.🧰 Tools
🪛 LanguageTool
[typographical] ~27-~27: Two consecutive dots
Context: ...nsettings.json. - Defaults to..(parent directory). - *Online Mode (...(DOUBLE_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/build-windows-executable-app.yaml(3 hunks).github/workflows/ci.yml(1 hunks).streamlit/config.toml(2 hunks).streamlit/credentials.toml(0 hunks)content/file_upload.py(2 hunks)docs/installation.md(0 hunks)docs/user_guide.md(1 hunks)docs/win_exe_with_embed_py.md(2 hunks)tests/test_run_subprocess.py(0 hunks)tests/test_simple_workflow.py(0 hunks)
💤 Files with no reviewable changes (4)
- .streamlit/credentials.toml
- tests/test_run_subprocess.py
- tests/test_simple_workflow.py
- docs/installation.md
✅ Files skipped from review due to trivial changes (1)
- docs/win_exe_with_embed_py.md
🧰 Additional context used
🧬 Code Definitions (1)
content/file_upload.py (2)
src/common/common.py (4)
page_setup(116-288)v_space(419-434)show_table(493-514)tk_directory_dialog(617-636)src/fileupload.py (2)
load_example_mzML_files(72-86)copy_local_mzML_files_from_directory(38-69)
🪛 actionlint (1.7.4)
.github/workflows/build-windows-executable-app.yaml
213-213: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 LanguageTool
docs/user_guide.md
[typographical] ~27-~27: Two consecutive dots
Context: ...n settings.json . - Defaults to .. (parent directory). - *Online Mode (...
(DOUBLE_PUNCTUATION)
🔇 Additional comments (12)
.streamlit/config.toml (2)
1-6: Clean-Up of Outdated Configuration Options
The removal of the[browser]section and themaxUploadSizesetting from the[server]section simplifies the configuration file. This aligns the config with current documentation and reduces potential confusion from deprecated options.
12-23: Consistency of Theme Color Settings
The configuration forprimaryColorand the commented-out options for background and text colors are consistent and easy to follow. Just ensure that these settings remain in sync with the behavior described in your documentation to avoid any future discrepancies..github/workflows/ci.yml (1)
31-31:Details
✅ Verification successful
Verify that tests in the
tests/directory are no longer needed.The CI workflow has been changed to only run
test_gui.pyinstead of also including the tests in thetests/directory. This aligns with the documentation improvements, but we should verify that the tests in thetests/directory are indeed no longer needed or have been moved elsewhere.
🏁 Script executed:
#!/bin/bash # Check if there are tests in the tests/ directory that might need to be run ls -la tests/ echo "Counting test files in tests/ directory:" find tests/ -name "test_*.py" | wc -lLength of output: 244
CI Workflow Verification: Removal of 'tests/' Directory Confirmed
After verification, no
tests/directory or test files were found in the repository. This confirms that the change to run onlytest_gui.pyin the CI workflow is intentional and aligns with the updated documentation. No further action is required.docs/user_guide.md (1)
18-28: Great addition clarifying online and local mode differences!This new section clearly explains the differences between online and local modes, which will be very helpful for users to understand the behavior of the application in different contexts.
🧰 Tools
🪛 LanguageTool
[typographical] ~27-~27: Two consecutive dots
Context: ...nsettings.json. - Defaults to..(parent directory). - *Online Mode (...(DOUBLE_PUNCTUATION)
.github/workflows/build-windows-executable-app.yaml (2)
199-204: Good addition of artifact download step.Adding this step to download the OpenMS package as an artifact improves the workflow by ensuring the package is available for the build process.
262-262: Nice simplification of the batch file creation.The batch file creation has been simplified to a single line that starts the Streamlit app, which improves maintainability.
content/file_upload.py (6)
6-6: Good consolidation of imports.Consolidating the imports from
src.common.commoninto a single line improves code readability.
13-16: Well-structured tab creation for different modes.The tabs are now created more cleanly, with the "Example Data" tab added and the "Files from local folder" tab conditionally added only in local mode.
31-37: Great addition of an Example Data tab!Adding a dedicated tab for loading example data provides a more intuitive user experience, especially for new users who want to try the application with sample data.
58-62: Improved warning message formatting.The warning message about deselecting the "Make a copy of files" option has been reformatted for better readability while preserving the important information.
70-71: Streamlined DataFrame creation.The DataFrame creation for displaying mzML files has been simplified into a single line, improving code readability.
78-79: Simplified DataFrame concatenation.The concatenation of external files into the DataFrame has been streamlined, making the code more concise.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/build_app.md (1)
29-29: Path update matches actual implementation - improve readability.The update correctly reflects the changed file path structure, where
common.pyis now located within acommonsubdirectory.Consider adding the article "a" before "set of useful functions" for better grammatical correctness:
-The `src/common/common.py` file contains set of useful functions for common use (e.g. rendering a table with download button). +The `src/common/common.py` file contains a set of useful functions for common use (e.g. rendering a table with download button).🧰 Tools
🪛 LanguageTool
[uncategorized] ~29-~29: You might be missing the article “a” here.
Context: ...common use (e.g. rendering a table with download button). ## Modify the template to bui...(AI_EN_LECTOR_MISSING_DETERMINER_A)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
assets/OpenMS.pngis excluded by!**/*.pngassets/openms.icois excluded by!**/*.ico
📒 Files selected for processing (5)
app.py(1 hunks)docs/build_app.md(1 hunks)docs/toppframework.py(2 hunks)docs/user_guide.md(1 hunks)docs/win_exe_with_pyinstaller.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/win_exe_with_pyinstaller.md
🧰 Additional context used
🪛 LanguageTool
docs/build_app.md
[uncategorized] ~29-~29: You might be missing the article “a” here.
Context: ...common use (e.g. rendering a table with download button). ## Modify the template to bui...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
docs/user_guide.md
[typographical] ~42-~42: Two consecutive dots
Context: ...in settings.json. - Defaults to .. (parent directory). ## Downloading Re...
(DOUBLE_PUNCTUATION)
🪛 Ruff (0.8.2)
app.py
6-6: pyopenms imported but unused
Remove unused import: pyopenms
(F401)
🔇 Additional comments (13)
docs/build_app.md (1)
33-33: Path update is consistent with project structure.This correctly reflects the updated location of the common.py file in the project structure.
app.py (3)
3-4: Good practice: explicit json import added.Adding an explicit import for the json module is better than relying on implicit imports. This improves code readability and makes dependencies clear.
8-8: Improved code readability with spacing.Adding whitespace improves readability by separating logical sections of the code.
6-6:Details
❓ Verification inconclusive
Ignore the unused import warning for pyopenms.
While static analysis flags
pyopenmsas unused, the comment indicates it's required for Windows compatibility. This is a case where the import should be kept despite being flagged as unused.
🏁 Script executed:
#!/bin/bash # Check if there are any Windows-specific code paths that use pyopenms grep -r "if.*windows.*pyopenms" --include="*.py" .Length of output: 52
Windows Compatibility: Verify pyopenms Import Usage
The current import of
pyopenmsinapp.py(line 6) is intended for Windows compatibility despite being flagged as unused by static analysis. However, our initial grep for Windows-specific usage ofpyopenmsproduced no results. Please manually verify that the import is required for Windows-specific behavior (for example, side effects or compatibility patches) even if its usage isn’t directly visible in conditional code blocks.
- Location:
app.py(line 6)- Action: Confirm that
pyopenmsis necessary for Windows compatibility; if so, add or update an in-code comment to clarify its role for future maintainers.🧰 Tools
🪛 Ruff (0.8.2)
6-6:
pyopenmsimported but unusedRemove unused import:
pyopenms(F401)
docs/user_guide.md (1)
39-42: Enhanced workspace documentation improves clarity.The updated formatting clearly distinguishes between Online and Local modes, making it easier for users to understand the differences in workspace behavior. The additional details about workspace directory configuration in Local Mode are valuable for users.
Note: The
..notation in line 42 is correct as it refers to the parent directory in Unix-style path notation.🧰 Tools
🪛 LanguageTool
[typographical] ~42-~42: Two consecutive dots
Context: ...insettings.json. - Defaults to..(parent directory). ## Downloading Re...(DOUBLE_PUNCTUATION)
docs/toppframework.py (8)
31-31: Grammar correction: "setup" → "set up".This correction improves the grammatical accuracy of the documentation.
33-33: Updated workflow instructions with accurate file paths.The revised instructions correctly direct users to edit or extend files in the
src/workflowdirectory, providing specific examples likesimpleworkflow.pyormzmlfileworkflow.py. This aligns with the actual project structure.
39-44: Improved modularization documentation.This update clearly explains how workflow logic has been modularized across multiple files, providing specific examples of files that define different workflows. This helps users understand the project structure better.
46-46: Updated count of important members in Workflow class.The documentation now correctly states that there are five important members in the Workflow class, which matches the actual implementation.
56-56: Added documentation for the file_manager member.The addition of documentation for the
self.file_managermember is valuable and completes the description of all important members of the Workflow class. This helps users understand how to handle file types and create output directories.
68-68: Storage location clarification.This update clarifies that input files are stored in the active workspace directory rather than a generic workflow directory, which aligns with the PR objectives to fix documentation issues related to file structure.
70-70: Improved subdirectory naming explanation.The clarification about how subdirectory names are determined based on the key passed to the
self.ui.upload_widgetmethod provides important context for developers.
74-77: Enhanced readability with bullet points.Converting this section to a bullet point list improves readability and makes the components of the file upload page clearer.
|
@t0mdavid-m https://docs.google.com/document/d/15YIZpm-cBS3q9tkHYxcuAJJc0LOn_IS6CPFJxc5pg2o/edit?usp=sharing could you please review the gsoc proposal and add comments (feedback) to it |
|
|
|
Hi @t0mdavid-m 👋, I’ve followed the review comments and made the suggested changes. Additionally, I cross-checked the documentation against the current repository structure and references to ensure everything is accurate and up-to-date. All relevant sections have been revised accordingly, and the documentation is now aligned with the existing codebase and deployment architecture. |
There was a problem hiding this comment.
I think we should not remove this part of the documentation.
There was a problem hiding this comment.
I think the logos have been updated separately.
| > 💡 Simply set a name for the workflow and override the **`upload`**, **`configure`**, **`execution`**, and **`results`** methods in your **`Workflow`** class. | ||
|
|
||
| The `Workflow` class contains four important members, which you can use to build your own workflow: | ||
| There is no longer a single `content/6_TOPP-Workflow.py` file. Workflow logic has been modularized across files like: |
There was a problem hiding this comment.
The user does not need to know what existed at a previous point in time.
|
Closing due to inactivity. |
2 participants
#174
I reviewed all relevant files and made necessary changes to ensure the documentation aligns with the actual file structure. While verifying the TOPP Workflow Developer Guide, I found some problems mentioned in issue thread.
🔹 Key Changes:
✔ Verified all files and made minor improvements to align them with the documentation.
✔ Did not find major errors—the documentation is generally well-structured and aligns with the implementation.
✔ Reviewed the session maintenance and parameter-saving approach – it's well-implemented using JSON and executed efficiently.
✔ Suggested improving the README to provide more details, making it easier for beginners to contribute, especially in a Streamlit-based application.
🔹 The documentation is good overall, but adding more details would help beginners understand the workflow and contribute more effectively:)
Let me know if you need any additional changes! 😊
Summary by CodeRabbit