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

Track token scores #571

Merged
merged 4 commits into from
Feb 28, 2024
Merged

Track token scores #571

merged 4 commits into from
Feb 28, 2024

Conversation

KarelVesely84
Copy link
Contributor

Hello,
I modified the code, so that the per-token scores are accessible via python API
(ys_probs, lm_probs, context_scores).

This is tracked during the modified beam search decoding of online transduces,
and it is exported for the BestHypothesis.

Would you be interesting in having this functionality in the codebase ?
This can be used as confidences from the client side...

But I did not evaluate it yet, to see how "useful" these numbers are.
I am ready to do changes as well. I'd like to open the discussion...

Best regards
K. Veselý

@csukuangfj
Copy link
Collaborator

Would you be interesting in having this functionality in the codebase ?

Yes, it is a useful feature and will benefit some users who want to get confidence for each token.

In addition to the modified_beam_search, could you also update the greedy_search?

@KarelVesely84 KarelVesely84 changed the title [not-for-merge] Track token scores [WIP] Track token scores Feb 14, 2024
@KarelVesely84
Copy link
Contributor Author

Okay, i extended it also for the greedy_search.
I did not test it yet... I'll do it soon...

@KarelVesely84
Copy link
Contributor Author

KarelVesely84 commented Feb 15, 2024

Hi Fangyun @csukuangfj ,
now, the per-token confidences are accessible both for greedy_search and modified_beam_search
of transducer decoding.

I manually tested it with CPU compiled sherpa-onnx through python API.

In the GH test-workflow I see some segmentation fault for Online-CTC in linux-gpu test.
Is this because the OnlineRecognizerResult class was extended in my PR ?

Do you also run the tests locally to investigate ?
K.

@csukuangfj
Copy link
Collaborator

Thanks! I will have time after today. Will look into it tomorrow.

@csukuangfj
Copy link
Collaborator

I see the error.

It is a use-after-free error.

Please see my comments about the code


/// Helper for `OnlineRecognizerResult::AsJsonString()`
template<typename T>
const std::string& VecToString(const std::vector<T>& vec, int32_t precision = 6) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const std::string& VecToString(const std::vector<T>& vec, int32_t precision = 6) {
std::string VecToString(const std::vector<T>& vec, int32_t precision = 6) {


/// Helper for `OnlineRecognizerResult::AsJsonString()`
template<> // explicit specialization for T = std::string
const std::string& VecToString<std::string>(const std::vector<std::string>& vec,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const std::string& VecToString<std::string>(const std::vector<std::string>& vec,
std::string VecToString<std::string>(const std::vector<std::string>& vec,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please never return a temporary reference from a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, thank you.

My bad. I remember in some context returning a local object as a const reference worked, but here I did it badly.
https://stackoverflow.com/questions/13318257/const-reference-to-temporary-vs-return-value-optimization

Best, K.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember in some context returning a local object as a const reference worked

Yes, you are right. But in this case, the temporary reference is from the function return value of a temporary object.
Think twice, there are two indirections.

Suppose obj is a local variable in a function. It is totally fine to call

return obj

to return a const reference.

But it is not valid to invoke

return obj.someMethod();

since obj is destroyed when the function returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right.

The code is fixed now. I tested the JSON output from python, and it looks fine.

Also now the lm_probs and contexts scores are filled only if LM and ContextGraph are used.
Otherwise there are empty lists to save bandwidth.

Thank you for finding the bug,
Karel

@KarelVesely84 KarelVesely84 force-pushed the track_token_scores branch 4 times, most recently from 0ab7a40 to e442564 Compare February 27, 2024 06:06
@KarelVesely84 KarelVesely84 changed the title [WIP] Track token scores Track token scores Feb 27, 2024
@csukuangfj
Copy link
Collaborator

I see that you have removed WIP. Do you think it is ready for review and merge?

- for best path of the modified-beam-search decoding of transducer
- export un-scaled lm_probs (modified-beam search, online-transducer)
- polishing
@KarelVesely84 KarelVesely84 force-pushed the track_token_scores branch 2 times, most recently from 48e4a6a to 4955fe8 Compare February 27, 2024 13:15
@KarelVesely84
Copy link
Contributor Author

KarelVesely84 commented Feb 27, 2024

Yes, it is almost ready to be merged.
Just making sure that the linting check passes...

It is ready for the review.

@KarelVesely84
Copy link
Contributor Author

Ok, the workflow tests seem fine, including the linter style-check... It is ready...

@csukuangfj
Copy link
Collaborator

I will have a second look after dinner. Thanks!

Copy link
Collaborator

@csukuangfj csukuangfj left a comment

Choose a reason for hiding this comment

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

Thanks again!

Left some minor comments. Otherwise, it looks good to me.

std::ostringstream oss;
oss << "[ ";
std::string sep = "";
for (auto item : vec) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
for (auto item : vec) {
for (const auto i&tem : vec) {

LogSoftmax(p_logit, vocab_size); // renormalize probabilities,
// save time by doing it only for
// emitted symbols
float *p_logprob = p_logit; // rename p_logit as p_logprob,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
float *p_logprob = p_logit; // rename p_logit as p_logprob,
const float *p_logprob = p_logit; // rename p_logit as p_logprob,

@KarelVesely84
Copy link
Contributor Author

okay, the two suggestions are implemented.... thank you...

@csukuangfj
Copy link
Collaborator

Thanks!

@csukuangfj csukuangfj merged commit 38c072d into k2-fsa:master Feb 28, 2024
64 of 227 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