-
Notifications
You must be signed in to change notification settings - Fork 212
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
Keep exceptions logs #898
Keep exceptions logs #898
Conversation
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.
Very nice draft @ekatef :D Indeed very needed too!
I think we have all the ingredients for this cake :D
May be good to finalize it for build_shapes and then add it to all scripts :)
scripts/_helpers.py
Outdated
tb = exc_traceback | ||
while tb.tb_next: | ||
tb = tb.tb_next | ||
flname = tb.tb_frame.f_globals.get("__file__") | ||
funcname = tb.tb_frame.f_code.co_name | ||
|
||
logger.error( | ||
"An error happened in module %r, function %r: %s", | ||
flname, | ||
funcname, | ||
exc_value, | ||
exc_info=(exc_type, exc_value, exc_traceback), | ||
) |
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.
This may be simplified with something like
def handle_exception(exc_type, exc_value, tb):
...
[Exclude Interrupt]
...
logger.exception(
"Exception {0}:{1} has occurred in the workflow.".format(exc_type, exc_value),
exc_info=True, stack_info=True
)
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.
Mmm on second though, I'd log also the keyboardinterrupt so to keep track that it has been a manual action
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.
Absolutely agree on keeping info on KeyBoardInterrupt. Added a functionality for that.
scripts/build_shapes.py
Outdated
@@ -36,8 +37,7 @@ | |||
from shapely.validation import make_valid | |||
from tqdm import tqdm | |||
|
|||
logger = logging.getLogger(__name__) | |||
logger.setLevel(logging.INFO) | |||
sys.excepthook = handle_exception |
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.
This shall be placed into the create_logger fuction but here we shall create the local logger like:
logger = create_logger(__name__, logging.INFO)
821c082
to
337fd78
Compare
Playing with structuring the code. It appears that python logging is a bit tricky to deal with due logger hierarchy,. The current implementation results in an empty log, but works if definition of |
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.
Cool @ekatef :D
Few minor comments, I think we are almost there :D
scripts/_helpers.py
Outdated
def create_logger(level=logging.INFO): | ||
# logger = logging.getLogger('__main__.' + __name__) | ||
logger = logging.getLogger("__main__") |
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.
I think we are quite close :D
I'd add a first mandatory input to this function like in the following.
Then, it can be used flexibly to create arbitrary loggers. This may solve the issue you mentioned in build shapes.
Could you test if it works?
def create_logger(level=logging.INFO): | |
# logger = logging.getLogger('__main__.' + __name__) | |
logger = logging.getLogger("__main__") | |
def create_logger(logger_name, level=logging.INFO): | |
logger = logging.getLogger(logger_name) |
Finally, docstrings may be recommendede to add, even simple ones with no ful description of inputs.
I'd say that the create_logger shall have a proper docstring, while handle_exception can have a simpler one
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.
I think we are quite close :D
I'd add a first mandatory input to this function like in the following. Then, it can be used flexibly to create arbitrary loggers. This may solve the issue you mentioned in build shapes. Could you test if it works?
Finally, docstrings may be recommendede to add, even simple ones with no ful description of inputs. I'd say that the create_logger shall have a proper docstring, while handle_exception can have a simpler one
It works!!!
Thank you so much :D
Adding docstrings :)
Agree, it looks like the issue has been fixed :) Thanks a lot for support, as always. Have added an exception hook to all the scripts, except |
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 are there :)
May you share few logs to me? I'd like to see a build_shapes (example) and build_renewable_profile if possible.
Many thanks!
scripts/add_electricity.py
Outdated
from powerplantmatching.export import map_country_bus | ||
|
||
idx = pd.IndexSlice | ||
|
||
logger = logging.getLogger(__name__) | ||
create_logger(__name__) | ||
sys.excepthook = handle_exception |
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.
Just seen; can't we place this line as well into create_logger?
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.
It appears, we perfectly can with the current logger definition. Great :D
scripts/_helpers.py
Outdated
""" | ||
Customise errors traceback. | ||
""" | ||
|
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.
Please, drop the empty line for PEP8 standard
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.
Fixed.
scripts/_helpers.py
Outdated
""" | ||
Create a logger for a module and adds a handler needed to capture in logs | ||
traceback from exceptions emerging during the workflow. | ||
""" | ||
|
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.
Please, drop the empty line after the """ for PEP8 standard.
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.
Done.
Have improved a definition of Sharing the logs bellow. build_shapes
build_renewable_profiles (onwind)
|
Uncaught exceptions are also captured by the logs. For example, there is a log after an error provoked in
|
a4dbf5a
to
64388a3
Compare
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.
Fantastic @ekatef :D From now on, logging will be easier! :D
Ready to merge!
Closes #897
Changes proposed in this Pull Request
Add an exception hook to stream exception traceback into the logs.
Checklist
envs/environment.yaml
anddoc/requirements.txt
.config.default.yaml
andconfig.tutorial.yaml
.test/
(note tests are changing the config.tutorial.yaml)doc/configtables/*.csv
and line references are adjusted indoc/configuration.rst
anddoc/tutorial.rst
.doc/release_notes.rst
is amended in the format of previous release notes, including reference to the requested PR.