Skip to content
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

Dev logging #304

Merged
merged 48 commits into from Apr 5, 2022
Merged

Dev logging #304

merged 48 commits into from Apr 5, 2022

Conversation

BeeGass
Copy link
Contributor

@BeeGass BeeGass commented Mar 2, 2022

Was hoping to get feedback on logging and ideally get a checklist of things that might be doing completely wrong, missing or need to still be done. Thanks

jacob-rosenthal and others added 15 commits February 7, 2022 12:33
Upon repeating installations to follow up to #296 and #297, I found that installing numpy first and using --no-cache-dir is the safest and most reliable way to install pathml
…he project. Also encorporated the ability to turn 'dev mode' on/off so that logging for pathml isnt done for users using the library. Work still needs to be done in logging more of the project
…ing before with how levels, filters and how they work with handlers within loguru. That has since been remedied
…are now being generated but still testing to have everything fully functional.
…d'ing into it. Also changed the notebook to include the images to download.
@jacob-rosenthal
Copy link
Collaborator

jacob-rosenthal commented Mar 2, 2022

Awesome! Here are some thoughts:

  • Consolidate to use a single log file
  • No need to have module-specific logging
  • Add tests
  • Move from submodule (pathml.logging) to just a file (e.g. pathml/_logging.py)
  • Add Dask config logging
  • Add a function that can be called to enable/disable pathml logs from within pathml (e.g. pathml.enable_logging(True)) this should be fully documented
  • Revert adding decorator to every function. Let's use logging calls inside functions instead.
  • Add dependencies to setup.py, environment.yml, and dockerfile
  • Save logs to a consistent filepath (not relative to current working directory)
  • Remove logs that were committed in examples directory

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2022

Codecov Report

Merging #304 (bf9fbb0) into dev (278e4a0) will decrease coverage by 0.38%.
The diff coverage is 65.30%.

@@            Coverage Diff             @@
##              dev     #304      +/-   ##
==========================================
- Coverage   86.88%   86.50%   -0.39%     
==========================================
  Files          26       27       +1     
  Lines        2447     2526      +79     
==========================================
+ Hits         2126     2185      +59     
- Misses        321      341      +20     
Impacted Files Coverage Δ
pathml/preprocessing/__init__.py 100.00% <ø> (ø)
pathml/preprocessing/transforms.py 88.81% <14.28%> (+0.01%) ⬆️
pathml/datasets/pannuke.py 88.51% <33.33%> (+0.07%) ⬆️
pathml/core/slide_data.py 85.04% <50.00%> (-0.34%) ⬇️
pathml/datasets/deepfocus.py 53.03% <50.00%> (+0.72%) ⬆️
pathml/ml/utils.py 59.74% <50.00%> (+0.52%) ⬆️
pathml/_logging.py 60.60% <60.60%> (ø)
pathml/core/slide_backends.py 91.42% <66.66%> (-1.79%) ⬇️
pathml/core/h5managers.py 81.97% <75.00%> (+0.10%) ⬆️
pathml/__init__.py 100.00% <100.00%> (ø)
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 278e4a0...bf9fbb0. Read the comment docs.

@BeeGass BeeGass marked this pull request as ready for review March 28, 2022 19:14
Copy link
Collaborator

@jacob-rosenthal jacob-rosenthal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ready for merge.
The logging functionality is implemented and works well in my testing locally. We also added a test to make sure that it is working as expected. Also went through and added logs to the backends, as well as logging dask cluster configs when using the default cluster. Also added documentation.

We can add logs now as we go, during development/debugging

@jacob-rosenthal jacob-rosenthal merged commit eb122b6 into dev Apr 5, 2022
@jacob-rosenthal jacob-rosenthal deleted the dev-logging branch April 5, 2022 20:46
@jacob-rosenthal jacob-rosenthal mentioned this pull request Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants