-
Notifications
You must be signed in to change notification settings - Fork 129
Add _logger.debug statements to cmodule default blas ldflags #528
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
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.
flake8 or so taught me to do it like this..
pytensor/link/c/cmodule.py
Outdated
@@ -2718,6 +2718,7 @@ def check_required_file(paths, required_regexs): | |||
found = True | |||
break | |||
if not found: | |||
_logger.debug(f"Required file {req} not found") |
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.
_logger.debug(f"Required file {req} not found") | |
_logger.debug("Required file '%s' not found", req) |
pytensor/link/c/cmodule.py
Outdated
check_mkl_openmp() | ||
except Exception as e: | ||
_logger.debug(e) | ||
_logger.debug(f"The following blas flags will be used {res}") |
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.
_logger.debug(f"The following blas flags will be used {res}") | |
_logger.debug("The following blas flags will be used: '%s'", res) |
return res | ||
else: | ||
_logger.debug(f"Supplied flags {res} failed to compile") |
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.
_logger.debug(f"Supplied flags {res} failed to compile") | |
_logger.debug("Supplied flags '%s' failed to compile", res) |
pytensor/link/c/cmodule.py
Outdated
_logger.debug("Will search for BLAS libraries in the following directories:") | ||
_logger.debug("{}".format("\n".join(searched_library_dirs))) |
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.
_logger.debug("Will search for BLAS libraries in the following directories:") | |
_logger.debug("{}".format("\n".join(searched_library_dirs))) | |
search_dirs = "\n".join(searched_library_dirs) | |
_logger.debug("Will search for BLAS libraries in the following directories:\n%s", search_dirs) |
Do you really want to use old style string interpolation instead of the newer f-strings? |
It's because this way, if there's an error it gets logged properly |
Out of curiosity, I looked a bit into why one would want to use old style string interpolation in logs. It seems like both formats will work fine and log properly, but the one you are suggested is only interpolated when the message is actually logged, while the modern interpolations are performed regardless of whether the message needs to be logged or not. I'll apply your suggestions and merge this. |
9f1bbef
to
db41c43
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #528 +/- ##
==========================================
+ Coverage 80.82% 80.84% +0.01%
==========================================
Files 162 162
Lines 46194 46269 +75
Branches 11288 11320 +32
==========================================
+ Hits 37337 37404 +67
- Misses 6630 6637 +7
- Partials 2227 2228 +1
|
Thanks for the explanation! I installed from this branch into my Docker image. The logger didn't show right away, and I had to set its level even though I had previously done a
|
With a bit more detailed logging (https://github.com/michaelosthege/pytensor/tree/mode-cmodule-logging)
|
closes #527