-
Notifications
You must be signed in to change notification settings - Fork 140
WIP - Feat/ main logger #640
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
Conversation
…e or console output.
`getLogger` got splitted in several functions to simplify reading. Added documentation at the head of the file.
|
|
||
|
|
||
| def setup_logger(loglevel='INFO', log_file=True): | ||
| def setup_logger(loglevel='INFO', log_file=True, mapdl_instance=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might not need this function, we can probably setup the logger elsewhere. I just wanted to keep backward compatibility in the code by adapting, more than changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping this in, but we can eventually remove this once this API change has settled.
akaszynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall you're on the right track and I think we're close.
A few notes:
- Great documentation in
logging.py. Clean it up to userstformatting. Right now it's a mix of markdown and rst. If we can get that cleaned up we can just add it directly to our docs. - Rename global logger to
pymapdl_global. If for whatever reason someone else wanted to use a global logger with the same name we'd conflict. It's a rare but non-zero possibility. - Rename
PyansysLoggertoLogger. I think we're safe to simply call it that, and it's a bit easier to type. - There must be a way to get the instance name outside of a getter. I feel like it's something that confuses the API a bit, and it would be better to have it as an attribute or a property (private if needed).
germa89
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review comments.
|
|
||
|
|
||
| def setup_logger(loglevel='INFO', log_file=True): | ||
| def setup_logger(loglevel='INFO', log_file=True, mapdl_instance=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might not need this function, we can probably setup the logger elsewhere. I just wanted to keep backward compatibility in the code by adapting, more than changing.
|
Ok solved. I'm going to point the Adapter to a new function "get_name", hence it will be force to be updated everytime the adapter is called. class PymapdlCustomAdapter(logging.LoggerAdapter):
def process(self, msg, kwargs):
kwargs['extra'] = {}
# This are the extra parameters sent to log
kwargs['extra']['instance_name'] = self.get_name() #instead of self.extra['name']
return msg, kwargsHence we do not need to use |
Removed the __getitem__ method to use 'get_mame'.
|
One error lead to another and I had to make a few changes to get this green. First, Python 3.7 vs. Python 3.8. Apparently there are different parameters in the logger:
Additionally, I ended up having to add in |
akaszynski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. That was another long one. We're ready for release.
Feel free to merge, and I'll release Friday.
|
Double checked on Windows, everything seems fine. Let's merge! |

logmoduleObjective
This module intends to create a general framework for logging in Pyansys.
This module is built upon
logginglibrary and it does NOT intend to replace it rather provide a way to interact betweenloggingandPyansys.The loggers used in the module include the name of the instance which is intended to be unique.
This name is printed in all the active outputs and it is used to track the different MAPDL instances.
Usage
Global logger
There is a global logger named
_Global_which is created atansys.mapdl.core.__init__.If you want to use this global logger, you must call at the top of your module:
You could also rename it to avoid conflicts with other loggers (if any):
To log using this logger, just call the desired method as a normal logger.
By default,
_Global_does not output to file or stdout. You should activate it from__init__.Instance logger
Every time that the class
_MapdlCoreis instantiated, a logger is created and stored in two places:_MapdlCore._log. For backward compatibility.LOG._instances. This field is adictwhere the key is the name of the created logger.These instance loggers inheritate the
_Global_output handlers and logging level unless otherwise specified.You can use this logger like this:
Other loggers
You can create your own loggers using python
logginglibrary as you would do in any other script.There shall no be conflicts between these loggers.
Notes
To-Do's
EDITED