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

Add perf metrics support for WhisperStaticPipeline #1337

Merged
merged 7 commits into from
Dec 23, 2024

Conversation

eshiryae
Copy link
Contributor

@eshiryae eshiryae commented Dec 6, 2024

No description provided.

@TolyaTalamanov
Copy link
Collaborator

@as-suvorov could you have a look, please?

@as-suvorov
Copy link
Contributor

I think we should add features_extraction_durations as well to be aligned with stateful pipeline. Example

@as-suvorov
Copy link
Contributor

as-suvorov commented Dec 10, 2024

There is also non segment tokens filtering implemented for stateful pipeline. I think this should also be added to the static pipeline.

@github-actions github-actions bot added the category: whisper Whisper pipeline label Dec 15, 2024
void infer_with_perf_metrics(ov::InferRequest& request, ov::genai::RawPerfMetrics& raw_metrics);

template <typename T>
void filter_by_ranges(std::vector<T>& value, size_t offset, std::vector<std::pair<size_t, size_t>>& ranges);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems filter_by_ranges used only in whisper_ustils.cpp. If you don't want to use it somewhere else it might make sense do not expose it to ov::genai::utils namespace. You can use unnamed namespace in whisper_utils.cpp (example: https://github.com/openvinotoolkit/openvino.genai/blob/master/src/cpp/src/whisper/whisper.cpp#L23).

Copy link
Collaborator

@TolyaTalamanov TolyaTalamanov left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Only minor comments!


namespace {

template <typename T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, in doesn't have sense to put templated function into anonym namespace, does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -0,0 +1,46 @@
// Copyright (C) 2023-2024 Intel Corporation
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe just 2024

filter_by_ranges(raw_metrics.m_batch_sizes, offset, ranges);
}

} // namespace utils
Copy link
Collaborator

Choose a reason for hiding this comment

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

identation is broken

@dmatveev dmatveev added this to the 2025.0 milestone Dec 17, 2024
@dmatveev dmatveev enabled auto-merge December 17, 2024 14:17
@dmatveev dmatveev added this pull request to the merge queue Dec 23, 2024
Merged via the queue into openvinotoolkit:master with commit 3496d45 Dec 23, 2024
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants