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

Issue 464 logger instead of print #499

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

opsdep
Copy link
Contributor

@opsdep opsdep commented Mar 3, 2021

No description provided.

@opsdep opsdep marked this pull request as ready for review March 3, 2021 08:20
Copy link
Member

@aperrin66 aperrin66 left a comment

Choose a reason for hiding this comment

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

You need to choose one way to configure logging:

  • either use the current mechanism with logger attributes defined for each object using nansat.utils.add_logger
  • or use a global logging configuration initialized in nansat.__init__

@opsdep
Copy link
Contributor Author

opsdep commented Mar 4, 2021

You need to choose one way to configure logging:

  • either use the current mechanism with logger attributes defined for each object using nansat.utils.add_logger
  • or use a global logging configuration initialized in nansat.__init__

No we need both of them because the run can be started with nansat usage as standalone (in that case, we need nansat.__init__ ), and as the second case when we run the harvesting, the nansat.utils.add_logger function configures the logger when it is needed.

@opsdep opsdep requested a review from aperrin66 March 4, 2021 07:53
@aperrin66
Copy link
Member

You need to choose one way to configure logging:

  • either use the current mechanism with logger attributes defined for each object using nansat.utils.add_logger
  • or use a global logging configuration initialized in nansat.__init__

No we need both of them because the run can be started with nansat usage as standalone (in that case, we need nansat.__init__ ), and as the second case when we run the harvesting, the nansat.utils.add_logger function configures the logger when it is needed.

It works exactly the same whether you use nansat alone or with geospaas_harvesting: the "Nansat" logger is created when nansat is imported, then the log level is overridden when add_logger is called (for example when a Nansat object is instantiated).

If you are going to use add_logger, you might as well put the whole logging configuration in there. It will be less confusing than having the configuration in logging.yml and then overriding the log level when add_logger is called.

@opsdep
Copy link
Contributor Author

opsdep commented Mar 4, 2021

@aperrin66 done

Copy link
Member

@akorosov akorosov left a comment

Choose a reason for hiding this comment

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

Good!
But I don;t see that logging.config, os.path or yaml is used in __init__.py why did you add that?
If it is not used, can you remove it?

Comment on lines +210 to +211
formatter = logging.Formatter('%(asctime)s - %(name)s - %(threadName)s - '
'%(levelname)s - %(message)s')
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the threadName field, nansat is single-threaded

Copy link
Member

Choose a reason for hiding this comment

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

actually is there any need to change the format at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to make it in a unified format when we start the process from harvesting, this helps to read the logs easily,all levels of nansat,harvesting and run_container.py MUST have same format of log. Because they are unified when call from each other. Reading logs with different format is difficult. so it is better to keep it. The thread is unknown, or sth like that,it does not matter, let it be.

nansat/nansat.py Outdated
@@ -1585,7 +1585,7 @@ def _import_mappers(log_level=None):
value: class Mapper(VRT) from the mappers module

"""
logger = add_logger('import_mappers', logLevel=log_level)
logger = add_logger('Nansat', logLevel=log_level)
Copy link
Member

Choose a reason for hiding this comment

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

this function is not part of the Nansat class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it is in nansat repository.DO you have any other name in your mind?

Copy link
Member

Choose a reason for hiding this comment

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

Either leave it as it was or use something like nansat.import_mappers.

Note: nansat refers to the package. Nansat refers to the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


from nansat.exceptions import WrongMapperError
from nansat.geolocation import Geolocation
from nansat.vrt import VRT

LOGGER = logging.getLogger("Nansat."+__name__)
Copy link
Member

Choose a reason for hiding this comment

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

__name__ already contains the full name of the module (here, it isnansat.mappers.mapper_aapp_l1b), so there is no need to prefix it with "Nansat.".
Same thing for the other mappers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right! But, when we run it from harvesting, only mapper_aapp_l1b is appear! It is better to keep that in the name of the lagger,isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Then write the full module name (nansat.mappers.mapper_aapp_l1b), that way there is no ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

3 participants