Make FFmpeg log level configurable #2439
Conversation
There was a problem hiding this comment.
utility function looks good, just want to double check default log value -- description mentions 0 but it is 8 in the code. generally enabling it for errors (16) by default makes more sense to me, but do you find this to be too noisy?
we can ask users to enable it when they encounter an issue
is there a good place we could add this message upon failure so users are aware of this log setting utility
torchaudio/utils/ffmpeg_utils.py
Outdated
| Arguments: | ||
| level (int): Log level. The larger, the more verbose. | ||
|
|
||
| The following values are valid values, the corresponding ``ffmpeg``'s |
There was a problem hiding this comment.
what happens if they enter an invalid value? should we add a check?
There was a problem hiding this comment.
I think they will boil down to a simple level comparison, so it's safe to put other values.
https://ffmpeg.org/doxygen/4.1/log_8c_source.html#l00316
It's just normally, log-levels are set by ffmpeg CLI, which exposes the listed values. Let me rephrase, common values, instead of saying valid values.
Description also says 8. In test code it's reset to 0.
16 can repeat messages across decoding units, so, decoding one audio file can still flood the log.
Decoder errors are not necessarily failure, that's why it's tricky. You would still get the tensor and keep going. But if you are training a model with many files, then the size of the training log could blow up with the decoding issues that is not an issue for DL practitioners. |
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Description also says 8.
I meant it says 0 in the PR description, but yea code looks good if 8 is intended.
For the second point, was more wondering if there was a good spot in the ffmpeg documentation or logging output to let users know that this logging functionality exists (this is not blocking)
| except Exception: | ||
| _IS_FFMPEG_AVAILABLE = False | ||
| return _IS_FFMPEG_AVAILABLE | ||
| return torchaudio._extension._FFMPEG_INITIALIZED |
There was a problem hiding this comment.
does this change relate to log level configurability?
There was a problem hiding this comment.
not technically. but as I write the test, I realized that the logic is duplicated in library and test util.
Oh my bad, fixed the description.
Good point, let me update the doc. |
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
I thought about it, but unlike the |
Summary: and set the default level to 8 for less verbose logging. Undesired logs are one of the loudest UX complains we get. Yet, loading media files involves uncertainty which is difficult to debug without debug log. This commit introduces utility functions to configure logging level so that we can ask users to enable it when they encounter an issue, while defaulting to non-verbose option. Pull Request resolved: pytorch#2439 Reviewed By: xiaohui-zhang Differential Revision: D36903763 Pulled By: mthrok fbshipit-source-id: ed495641a868ed9a3f34c2a5f37fa3e5b135d0ec
|
This pull request was exported from Phabricator. Differential Revision: D36903763 |
|
@carolineechen let me revert the default log level modification, as it might be disruptive if other libraries / existing code base expects log level with already set. |
|
@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
Hey @mthrok. |
and set the default level to 8 for less verbose logging.
Undesired logs are one of the loudest UX complains we get.
Yet, loading media files involves uncertainty which is
difficult to debug without debug log.
This commit introduces utility functions to configure logging level
so that we can ask users to enable it when they encounter an issue,
while defaulting to non-verbose option.