-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[MetaSchedule] Logging Interface Unification #11157
[MetaSchedule] Logging Interface Unification #11157
Conversation
Thanks for massively enhancing the current logging system! However, on the implementation, we do need to follow the standard best practice in python, namely, using: logger = logging.getLogger(__name__) to get a logger for this file. |
Thanks for the careful review, I've fixed all the logger usage and made sure it follows the current recommended practice. Ready for review. |
python/tvm/meta_schedule/tune.py
Outdated
@@ -17,7 +17,8 @@ | |||
"""User-facing Tuning API""" | |||
# pylint: disable=import-outside-toplevel | |||
import logging | |||
import os.path | |||
from pathlib import Path |
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.
Pathlib is definitely a good library, but perhaps it's less plausible if we mix os.path
and Pathlib
. In our particular case, shall we just do:
os.makedirs(path, exist_ok=False)
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 it might be acceptable to use exist_ok = True
in this case cause the logs folder could be reused.
d49fa1f
to
feb2bfe
Compare
@@ -32,12 +32,14 @@ class CrossThreadReductionNode : public ScheduleRuleNode { | |||
Optional<Integer> opt_warp_size = target->GetAttr<Integer>("thread_warp_size"); | |||
|
|||
if (!opt_max_threads_per_block.defined()) { | |||
LOG(WARNING) << "Target does not have attribute \"max_threads_per_block\", therefore the " | |||
"rule CrossThreadReduction will not be applied"; | |||
TVM_PY_LOG(WARNING, context->logging_func) |
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 was thinking if it's better to make it a LOG(FATAL) instead, but it's do that in a separate PR
e9a1d8c
to
2863996
Compare
Any updates? |
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.
LGTM! Please try it out for the last time and confirm it prints the dates properly, and then feel free to merge it yourself
Thanks @junrushao1994 @Hzfengsy for reviewing! |
* Implement new logging interface. * Major interface usage update. * Functionality fix. * Switch logging conditions. * Tweak logging interface. * Minor fix. * Feature updates. * Logging usage. * Linting. * Fix linting. * Fix handler type. * Fix issues. * Nits. * Address issues. * Add DEBUG level fall back. * Minor fixes. * Allow parameterized configuration. * Linting. * Polish interface.
* Implement new logging interface. * Major interface usage update. * Functionality fix. * Switch logging conditions. * Tweak logging interface. * Minor fix. * Feature updates. * Logging usage. * Linting. * Fix linting. * Fix handler type. * Fix issues. * Nits. * Address issues. * Add DEBUG level fall back. * Minor fixes. * Allow parameterized configuration. * Linting. * Polish interface.
This PR introduces a new interface to unify meta schedule logging from both c++ and python side. It will be easier to control logging level and format content. From now on, only task scheduler level will be printed to screen by default, and task level information will be logged to a file in given folder. We still support any logging configuration from outside, and use
tvm.meta_schedule
as the main logger.The interface uses logger on the python side while on the c++ side we created a new macro called
TVM_PY_LOG
to work int the following style wherelogging_func
is the optional packed function for logging (thanks to #10032 ):The logger would fall back to original c++ logger function
LOG(Level)
if given logging function is not defined.Recomended setup of logger is as follows:
And further configuration of task level loggers can be set in
TuneConfig
for features like whether to stream to screen, whether to propagate to main logger, patterns, etc, by passing a Dict toTuneConfig
as follows:Default setting is to output to files in
log_dir
folder and the logging structure looks like the following directory:Suggestions are welcome!
CC: @junrushao1994 @shingjan @comaniac