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

[POC] use logging module #565

Closed
7 tasks
adebardo opened this issue Aug 22, 2024 · 3 comments · Fixed by #610
Closed
7 tasks

[POC] use logging module #565

adebardo opened this issue Aug 22, 2024 · 3 comments · Fixed by #610
Labels
[POC] Conception To review Tickets needs approval about it conception

Comments

@adebardo
Copy link

adebardo commented Aug 22, 2024

Context

When using xdem errors and other types of information are displayed via print. The use of the logging module is more relevant, especially with the arrival of the CLI. This ticket, therefore proposes to replace all prints of xdem in logging.

First, it is good to read the [documentation] (https://docs.python.org/en/3/howto/logging.html) in order to master the different levels and constraints related to the use

Code

The files affected by the changes are as follows:
-xdem/examples.py

  • xdem/affine.py
    -xdem/base.py
    -xdem/biascorr.py
    -xdem/workflows.py

  • xdem/fit.py
    -xdem/misc.py
    -xdem/spatialstats.py

  • xdem/volume.py

  • example/plot_nuth_kaab.py

  • example/plot_block_wise_coreg.py

  • example/plor_deramp.py

  • example/plot_heterosc_estimation_modelling.py

  • example/plot_standardization.py

  • example/plot_variogram_estimation_modelling.py

  • example/plot_dem_substraction.py

  • example/plot_infer_heterosc.py

  • example/plot_infer_spatial_correlation.py

  • example/plot_spatial_error_propagation.py

  • test/test_coreg_pipeline.py

  • test/test_demcollection.py

  • test/test_doc.py

  • In xdem at the moment there is a verbose variable that if enabled then prints are displayed. Therefore, this variable must be removed from the APIs.

  • replace the prints with adequate logging.debug, logging.info, etc.
    ⚠️ f-string doesn’t pass pylint with logging we recommand using the percent formatting. (%s :string, %d int, %f float, ...)

  • Add some logs in run and tests it

For CLI

Doc

  • Add a paragraph to tell the user how to enable this verbosity in CLI mode but also in API mode

Some help for an API use

import logging  
  
# Configuration 
logging.basicConfig(level=logging.DEBUG,  
                    format='%(asctime)s - %(name)s - %(levelname)s - %(message)s',  
                    datefmt='%Y-%m-%d %H:%M:%S',  
                    handlers=[  
                        logging.FileHandler('app.log'),  # saving in a file  
  logging.StreamHandler()  # Print in console  
  ])  
  
  
logger = logging.getLogger('mylogger')  
  
def division(a, b):  
    try:  
        result = a / b  
  except ZeroDivisionError:  
        logger.error("Error : division by zero")  
    else:  
        logger.info("The %s / %s = %s", a, b, result)  
        return result  
  
division(10, 2)  
division(10, 0)

Tests

  • verify that the logging file from CLI has been created

Estimation 3d

@adebardo adebardo added the [POC] Conception To review Tickets needs approval about it conception label Aug 22, 2024
@duboise-cnes
Copy link
Member

Thanks @adebardo for the issue. It's ok for me globally.
Some remarks and complements from my experience of other projects:

  • for cli configuration, we could put it in the configuration file and not keep command line parameter if easier (to be
  • I agree to find a simple way to use from CLI AND API for user to change the level of debug
  • We have also added on CARS project a PROGRESS level which is not bad for information progress in pipeline CLI. Anyway, each level have to be well defined and keep logging standards is easier for end users/developers.
  • for many reasons (performance and compatibily), f-string is not so well in logging. Python recommends old %s way. Linter can be parameter on the choice made (pylint i know, for ruff to be found...) : https://docs.python.org/3/howto/logging.html#logging-variable-data . In demcompare we were using .format method. I think it is a good to have one at the conversion and keep it all along the code for consistency (with help of the linter, this can be added to the issue).
  • compared with the example, we were using directly logging.info, logging.debug much easier to not pass the "logger" throughout the modules.
  • I think we can rely and have inspiration on demcompare parts : https://gitlab.cnes.fr/3d/demcompare/-/blob/master/demcompare/log_conf.py?ref_type=heads and https://gitlab.cnes.fr/3d/demcompare/-/blob/master/demcompare/__init__.py?ref_type=heads#L92

@adebardo
Copy link
Author

adebardo commented Aug 30, 2024

Thank you for this feedback.
1 - I acknowledge the possibility of adding this to the config file indeed.
2 - I’ve noted the point about the logging format.
3 - I’ll also add more global logging management.

@adehecq
Copy link
Member

adehecq commented Sep 4, 2024

It would be great to have the logging option indeed and be able to set it up through the CLI and API. We can also possibly add it to a global config file like what is done in geoutils.

  • for many reasons (performance and compatibily), f-string is not so well in logging.

Just out of curiosity, what is wrong with the f-string? Why would it have lower performance? I got used to the f-strings now because they are much easier to read. So I would try to use f-string whenever possible, i.e. outside of logging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[POC] Conception To review Tickets needs approval about it conception
Projects
None yet
3 participants