Windows error #250
Windows error #250
Conversation
yusufuyanik1
commented
Jul 23, 2024
yusufuyanik1
commented
- Added warnings about Pandoc and pdstools version to Health Check app.
- Seperated generateReport function to generate_health_check and generate_model_reports
- Changed the temp_dir logic. Everything is written to a freshly created temp_dir and the whole folder is removed if we are not in debug mode.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #250 +/- ##
==========================================
- Coverage 59.75% 59.30% -0.45%
==========================================
Files 29 29
Lines 3809 3863 +54
==========================================
+ Hits 2276 2291 +15
- Misses 1533 1572 +39 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Some small comments, but thanks for this @yusufuyanik1, looks great overall!
|
|
||
|
|
||
| current_version = pdstools.__version__ | ||
| latest_version = get_latest_pdstools_version() |
There was a problem hiding this comment.
You probably want to cache this function?
python/pdstools/adm/ADMDatamart.py
Outdated
| self, | ||
| name: Optional[str] = None, | ||
| working_dir: Path = Path("."), | ||
| model_list: List[str] = [], |
There was a problem hiding this comment.
Initializing lists in the signature is typically a bad idea, I would initialize with None and generate an empty list in the function itself
python/pdstools/adm/ADMDatamart.py
Outdated
| delete_temp_files: bool = True, | ||
| output_type: str = "html", | ||
| allow_collect: bool = True, | ||
| cached_data: bool = False, | ||
| predictordetails_activeonly: bool = False, |
There was a problem hiding this comment.
maybe we should make this argument something like predictor_details_active_only? also modelid should be model_id I think?
| The working directory for the output. If None, uses current working directory. | ||
| output_type : str, default='html' | ||
| The type of the output file (e.g., "html", "pdf"). | ||
| debug_mode : bool, optional |
There was a problem hiding this comment.
Debug mode doesn't sound like the proper argument name for this description, wouldn't it be something like delete_temp_dir? debug mode sounds like you'd be logging more information while running
There was a problem hiding this comment.
We already have the verbose argument for logging
- Use verbose for both logging and keeping temp_dir
- Create a new argument, debug_mode which overwrites verbose
debug_mode = kwargs.get("debug_mode", self.verbose)
So when it is set to True, it keeps logs and doesn't delete temp_dir
There was a problem hiding this comment.
Thanks for the notes @StijnKas, now all your notes are addressed except this. Do you think this is fine or should we separate the verbose and delete temp_dir logic?
There was a problem hiding this comment.
Logically I don't think they're the same thing but I follow your logic and I think it's fine to make it easier on the user. Let's stick with this.
python/pdstools/adm/ADMDatamart.py
Outdated
| name = name.replace(" ", "_") if name else None | ||
| if report_type == "ModelReport": | ||
| if not modelid: | ||
| raise ValueError("ModelID is required for a ModelReport.") |
There was a problem hiding this comment.
Note the difference in capitalisation here: I would recommend making the argument name model_id everywhere, and then having model_id being the 'required' property in your value error as well
python/tests/test_healthcheck.py
Outdated
| sample_without_predictorbinning.generateReport( | ||
| name="MyOrg", modelid="bd70a915-697a-5d43-ab2c-53b0557c85a0" | ||
| sample_without_predictorbinning.generate_model_reports( | ||
| name="MyOrg", modelid=["bd70a915-697a-5d43-ab2c-53b0557c85a0"] |
There was a problem hiding this comment.
argument modelid taking a list of values doesn't make intuitive sense - is that right?
|
Resolves #213 |
| import streamlit as st | ||
|
|
||
| import pdstools | ||
| from pdstools.utils.streamlit_utils import get_latest_pdstools_version |
There was a problem hiding this comment.
Last remark: I don't think this should be in streamlit_utils, it's not streamlit specific
There was a problem hiding this comment.
Ah I see, you want it to be cached. Maybe it could be better to use some other caching mechanism so it's streamlit agnostic?
There was a problem hiding this comment.
Right, I used streamlit's caching decorator, will try to use another way of caching
There was a problem hiding this comment.
Carried the original function to cdh_utils and calling this from streamlit_utils now.
@st.cache_data
def st_get_latest_pdstools_version():
return cdh_utils.get_latest_pdstools_version()
