-
Notifications
You must be signed in to change notification settings - Fork 0
✨ make relative outputs and path work better in streamlit #146
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
…folder Now this works and can be run from the docs folder using: cd docs vuegen -c example_config_files/Basic_example_vuegen_demo_notebook_config.yaml -output_dir ../tests/report_examples/Basic_example_vuegen_demo_notebook_cfg streamlit run ../tests/report_examples/Basic_example_vuegen_demo_notebook_cfg/streamlit_report/sections/report_manager.py
- this should also bug in `split_readme.py` where the link was not updated, leading to a missing landing page!
- outputdir only really makes sense for report generation, not execution: In the current implementation the report has to execute from the path it was generated from - should the static dir be change upon change of section_dir?
- currently the section_dir is set with the knowleadge of the streamlit report structure - Allows to execute report from everywhere on that operating system - folder with streamlit report cannot be copied though.
- could also contain completion message?
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.
Pull Request Overview
This PR refactors how the generated Streamlit report handles relative paths by introducing a consistent section_dir, updating all section scripts to compute file paths via pathlib, and recording the report’s execution context.
- Introduce
sections_dirparameter andself.section_dirattribute inStreamlitReportView - Change all generated Python section files to import
Path, setsection_dir, and computeplot_file_path/file_pathvia(section_dir / …).resolve().as_posix() - Update CI workflow, docs, Makefile, and config files to point to the new
docs/images/logo/path and validate report generation in tests
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/vuegen/streamlit_reportview.py | Added sections_dir init param; use self.section_dir for relative imports and README |
| src/vuegen/report_generator.py | Pass sections_dir to view; drop explicit output_dir args |
| tests/report_examples/**/streamlit_report/sections//.py (all section scripts) | Import Path, define section_dir, and build file paths via section_dir |
| tests/report_examples/**/streamlit_report/static/Man_Example.html | No change in logic, static HTML moved under correct static dir |
| gui/Makefile | Updated pyinstaller add-data path to docs/images/logo/vuegen_logo.png |
| .github/workflows/cdci.yml | Added a step to regenerate and check Streamlit report |
| docs/vuegen_basic_case_study_configfile.md | Updated logo URL to .../docs/images/logo/... |
| docs/example_config_files/*.yaml | Updated logo and graphical_abstract URLs to new logo/ path |
| docs/split_readme.py | Pointed VueGen logo to new logo/ directory |
| docs/**/README.md and .ipynb | Updated image paths and notebook metadata versions |
Comments suppressed due to low confidence (3)
src/vuegen/streamlit_reportview.py:53
- [nitpick] The parameter name
sections_diris inconsistent with the attributeself.section_dir; consider renaming the parameter tosection_dirto match and avoid confusion.
sections_dir: str = SECTIONS_DIR,
src/vuegen/streamlit_reportview.py:92
- The docstring still refers to a default of
SECTIONS_DIR; update it to explain thatoutput_diris now optional and defaults to the configuredsection_dirwhenNone.
def generate_report(self, output_dir: str = None) -> None:
src/vuegen/report_generator.py:113
- [nitpick] Since
generate_reportno longer accepts an explicitoutput_dir, ensure callers relying onoutput_dircontinue to work; consider logging a warning if the CLI passed an--output_dirflag.
st_report.generate_report()
sayalaruano
left a 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.
It is looking good, thanks a lot for taking care of it :)
I just encountered a small bug. When I ran the streamlit app with the EMP directory, there is an indentation error with the description of the project on the Home page. It looks like this in the generated Python file:
st.markdown(
(
"<p style='text-align: center; "
"color: #000000;'>The Earth Microbiome Project (EMP) is a systematic attempt to characterize global microbial taxonomic and functional diversity for the benefit of the planet and humankind.
It aimed to sample the Earth’s microbial communities at an unprecedented scale in order to advance our understanding of the organizing biogeographic principles that govern microbial community structure.
The EMP dataset is generated from samples that individual researchers have compiled and contributed to the EMP.
The result is both a reference database giving global context to DNA sequence data and a framework for incorporating data from future studies, fostering increasingly complete characterization of Earth’s microbial diversity.
You can find more information about the Earth Microbiome Project at https://earthmicrobiome.org/ and in the [original article](https://www.nature.com/articles/nature24621).</p>"
),
unsafe_allow_html=True)
|
Also, I think that it would be nice to implement the code to identify if there is an image in the root folder of the data, and assign it to the graphical_abstract parameter. What do you think? Should we do this on this PR or create a new one? |
|
Yes we can add that, but I would do it in a new PR. |
This was actually a regression from the rewriting of markdown blocks using |
|
The changes are quite extended, so not too clutter this I will open a new PR only with the required changes for this specific error of multi-line description documents. |
| (default is SECTIONS_DIR). | ||
| """ | ||
| if output_dir is not None: | ||
| # ? does this imply changes to the static dir |
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.
Question: Should the static dir always be in the output folder?
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.
Yes, I think so, if there are interactive components when creating static reports, we save these plots in that folder, and read from it.
Now this works and can be run from the docs folder using:
cd docs vuegen -c example_config_files/Basic_example_vuegen_demo_notebook_config.yaml -output_dir ../tests/report_examples/Basic_example_vuegen_demo_notebook_cfg streamlit run ../tests/report_examples/Basic_example_vuegen_demo_notebook_cfg/streamlit_report/sections/report_manager.pySolution to be implemented for relative paths:
One or both options:
StreamlitReportViewclass to init. This will allow to make changes to the logic relatively easy.