kirbyju / tcia_utils

A package to simplify common tasks one might perform when interacting with The Cancer Imaging Archive (TCIA) via Jupyter/Python.
Apache License 2.0
14 stars 4 forks source link

Change most calls to print() to calls to _log.{level}() #5

Closed zjp closed 1 year ago

zjp commented 1 year ago

Closes #1

I think the logging levels chosen for the various print statements are sensible. To replicate the previous behavior, I added a logging.StreamHandler() and set its level to INFO.

Now other modules can get this module's logger and disable it if they don't want to see the messages.

kirbyju commented 1 year ago

I have done some more testing and have a few questions.

  1. When I import and run this from a notebook the output for everything is now shown twice (see below as an example with the getImage function). Is there some way to prevent this?
  2. Do you think it'd be worth adding timestamps to the output? It's probably not important to Jupyter users but people using this in other contexts might appreciate having that extra info.

Downloading 3 out of 612 Series Instance UIDs (scans). INFO:nbia:Downloading 3 out of 612 Series Instance UIDs (scans). Downloading... https://services.cancerimagingarchive.net/nbia-api/services/v1/getImage?NewFileNames=Yes&SeriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.5168.1900.104193299251798317056218297018 INFO:nbia:Downloading... https://services.cancerimagingarchive.net/nbia-api/services/v1/getImage?NewFileNames=Yes&SeriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.5168.1900.104193299251798317056218297018 Downloading... https://services.cancerimagingarchive.net/nbia-api/services/v1/getImage?NewFileNames=Yes&SeriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.5168.1900.614409775072665417753627581100 INFO:nbia:Downloading... https://services.cancerimagingarchive.net/nbia-api/services/v1/getImage?NewFileNames=Yes&SeriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.5168.1900.614409775072665417753627581100 Downloading... https://services.cancerimagingarchive.net/nbia-api/services/v1/getImage?NewFileNames=Yes&SeriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.5168.1900.125236013959219285601516346712 INFO:nbia:Downloading... https://services.cancerimagingarchive.net/nbia-api/services/v1/getImage?NewFileNames=Yes&SeriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.5168.1900.125236013959219285601516346712 Downloaded 3 out of 3 requested series from a total of 612 Series Instance UIDs (scans). 0 failed to download. 0 previously downloaded. INFO:nbia:Downloaded 3 out of 3 requested series from a total of 612 Series Instance UIDs (scans). 0 failed to download. 0 previously downloaded.

zjp commented 1 year ago

At first I wasn't able to reproduce this behavior, but I kept experimenting and found that if I imported the logging module and called basicConfig in a notebook, I could get duplicate output. On investigating, I could see that the StreamHandler in the notebook and the StreamHandler supposedly from tcia_utils had the same memory address.

I then tried taking out the StreamHandler, but that meant I had to import logging; logging.basicConfig in my notebook to see output. I considered that a regression, so then I tried calling basicConfig from tcia_utils. After that, the duplicate output stopped and I no longer had to use the logging module in my notebooks.

I also added timestamps. It's not hard, and it's good info.

kirbyju commented 1 year ago

I should've been more specific in my prior comments. My notebook testing was mostly being done in Google Colab. I just did some testing with your latest code and everything works for me in JupyterLab 3.4.4. On Google Colab it appears that the warnings and errors show up properly but none of the info statements appear unless you import logging in the notebook and then run something like this to get rid of Colab's default handler and set your own.

# Delete existing handlers
for handler in logging.root.handlers[:]:
    logging.root.removeHandler(handler)

# Set handler with level = info
logging.basicConfig(format='%(asctime)s:%(levelname)s:%(message)s', 
                    level=logging.INFO)

Would adding the code to delete existing handlers into nbia.py itself present problems for use cases outside of Colab? I think the other option is to modify my existing notebooks to run an extra cell to address this for people working from Colab.

kirbyju commented 1 year ago

I'm going to go ahead and merge these changes and update the PyPI release . If it turns out later that there's a better way to handle support in Colab we can do that, but it's not a huge issue and I'd rather people be able to benefit from your work sooner than later!

zjp commented 1 year ago

Sweet! I think that's a good workaround for Colab -- deleting handlers inside this module will definitely result in surprise for users outside that environment.