Skip to content
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

Fix LogLevels #9

Merged
merged 2 commits into from
Aug 17, 2024
Merged

Fix LogLevels #9

merged 2 commits into from
Aug 17, 2024

Conversation

AlexCharlton
Copy link
Contributor

Hello! I've been trying to use this library with suppressed logging for all but errors. I.e, in my program I had set:

vosk::set_log_level(vosk::LogLevel::Error);

Despite this setting, I was still getting warning messages, which were causing me some grief. Going to the Kaldi source, we can see that there's a misalignment between the LogLevel struct and the upstream understanding of log levels (perhaps due to upstream evolution over time). In particular, there's no such thing as a Debug level (i.e. sending a value of 1 is not meaningful), but there is Warn level (-1) before the Error level (-2). This PR addresses this discrepancy.

Now, this is a breaking change as we're redefining the contents of the LogLevel enum: adding a new level (Warn), removing Debug and renaming ErrorInfo to be simply Info. Due to the mismatch of the enum to the upstream, I don't think there's a reasonable way of making this a non-breaking change.

Not only is this a breaking change, but it's also altering the existing LogLevel naming convention. I'm open to changing this, but ErrorInfoDebug is an unusual name, differing from both Rust conventions for naming levels as well as the upstream Kaldi's conventions. The new names align with both of these standards.

@Bear-03
Copy link
Owner

Bear-03 commented Aug 4, 2024

Good catch! I used the documentation in vosh_api.h to write and document the log levels, but I checked the code behind set_log_level and it just calls Kaldi's SetVerboseLevel (link) directly so the behaviour should be the same...

Would you mind reporting this also to upstream? It'd be good if they fixed the documentation also in the header file there.

What I don't understand though, is the case of Java's bindings The enum has different values compared to those you propose, so I am not sure which ones it should be... We should probably just mimic the Severity enum but I still find it weird that the vosk team didnt do that.

@AlexCharlton
Copy link
Contributor Author

AlexCharlton commented Aug 4, 2024

Ah, yeah, I never looked at the vosk_api.h, just at set_log_level. Since the Java bindings are also just calling set_log_level, I can only assume they are also experiencing the wrong behavior. Maybe this is something that changed in Kaldi: I saw that at some point they refactored how they log things, but never looked at how it worked before. I'll report this to vosk-api 👍.

If you don't use the same values as the Severity enum when calling set_log_level, I can attest that you do get the wrong results. I didn't bother including the kAssertFailed value, however, because I couldn't tell what the real difference would be to a Vosk user if Kaldi throws an error versus aborts.

@AlexCharlton
Copy link
Contributor Author

Now that I look closer at the Java enum and the docs for vosk_api.h, I'm realizing nothing is technically incorrect, just misleading. vosk_api.h states that 0 will print info messages (true), less than 0 won't print info messages (true but vague) and that values greater than 0 is a "more verbose mode" (there's nothing more verbose than info in the case of Kaldi, so this is misleading but not strictly false). Same goes for the Java enum, which has a WARNINGS value that is correctly set to -1, just no ERROR value.

I'll still report upstream, though. No one benefits from this kind of sloppiness!

@Bear-03
Copy link
Owner

Bear-03 commented Aug 4, 2024

Fair enough! When you report this to the vosk team, make sure to link this issue so they know the context.

I'd rather wait to see if they clear things up, but otherwise this seems good to merge.

@AlexCharlton
Copy link
Contributor Author

Reported. Feel free to weigh in if you think anything I said is off.

If the upstream follows my suggested resolution, there will be no effective change to their API, so this PR should still be roughly the right thing to do. One thing that I just learned, however, is that there are debug logs, but it doesn't look like they're part of the public API. There are values of at least 5 used, evidently to control how debug logs are printed for tests. Because there doesn't seem to be much consistency to their application, I'm not sure I'd suggest adding debug logs to this API.

Copy link
Owner

@Bear-03 Bear-03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Judging by what we talked here and the reply you got from the vosk team, I think this PR is indeed the way to go. If you don't have any other suggestions I'm happy to merge this.

@AlexCharlton
Copy link
Contributor Author

Sweet, that sounds good to me.

FWIW, I see that the rustfmt and clippy failures seem to be due to some CI issue:

image

@Bear-03
Copy link
Owner

Bear-03 commented Aug 17, 2024

Yeah I saw, rustfmt seems to work now, probably a github CI error, clippy shows some lints but that's okay, I'll fix them myself later.

Merging now, thanks for the work!!

@Bear-03 Bear-03 merged commit 2ceb01a into Bear-03:main Aug 17, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants