Telemetry coverage extension #3039
Conversation
github-actions
bot
added
NNCF PT
nncf/common/strip.py
Outdated
|
|
||
| TModel = TypeVar("TModel") | ||
|
|
||
|
|
||
| @api(canonical_alias="nncf.strip") | ||
| @tracked_function(category=NNCF_COMMON_CATEGORY, extractors=[ModelProcessedWithStripApi()]) |
There was a problem hiding this comment.
I would suggest to use MODEL_BASED_CATEGORY.
nncf/telemetry/events.py
Outdated
| @@ -15,6 +15,9 @@ | |||
| from nncf.common.utils.backend import BackendType | |||
| from nncf.common.utils.backend import get_backend | |||
|
|
|||
| # General categories | |||
| NNCF_COMMON_CATEGORY = "nncf_common" | |||
There was a problem hiding this comment.
| NNCF_COMMON_CATEGORY = "nncf_common" | |
| NNCF_CATEGORY = "nncf" |
There was a problem hiding this comment.
It does make no sense in terms of the categorisation. For example, nncf_ov or nncf_onnx may be the subsets of the nncf category, but not nncf_common. The originally proposed category reflects the category's differences.
IMHO.
| extractors=[ | ||
| DatasetGeneratedFromApi(), | ||
| ], | ||
| ) | ||
| def generate_text_data( |
There was a problem hiding this comment.
Please mark this function with the api decorator.
nncf/common/telemetry_extractors.py
Outdated
|
|
||
| class ModelProcessedWithStripApi(TelemetryExtractor): | ||
| def extract(self, _: Any) -> CollectedEvent: | ||
| return CollectedEvent(name="model_processed", data="strip_api") |
There was a problem hiding this comment.
@KodiaqQ, @MaximProshin, @AlexKoff88 I would suggest to introduce function_call event to cover calling all NNCF functions instead of using named events, as following:
return CollectedEvent(name="function_call", data="nncf.strip")
I would like to know your opinion.
There was a problem hiding this comment.
Sounds reasonable to me.
In this case, we'll have one event with different data values.
The only question is whether it is possible to analyse such an event in the front end.
There was a problem hiding this comment.
Sounds reasonable to me. In this case, we'll have one event with different data values. The only question is whether it is possible to analyse such an event in the front end.
I will try to clarify this question.
There was a problem hiding this comment.
Any updates? Should we do something with this PR or just hold?
There was a problem hiding this comment.
Sounds reasonable to me. In this case, we'll have one event with different data values. The only question is whether it is possible to analyse such an event in the front end.
@popovaan, could you provide your opinion?
There was a problem hiding this comment.
I see no problems in usage of common function_call event name and it is convenient in terms of making a report, as all values can be collected using only one filter Event name==function_call.
If you want a separate group of function calls, which are needed to be a separate table in report (for example, function calls that raised error), you can introduce another event name to group them. For example: failed_function_call. Otherwise separate names per method are redundant.
There was a problem hiding this comment.
@alexsu52 what should we do here then?
|
Do we need this, @alexsu52, @MaximProshin? |
Changes
Reason for changes
Related tickets
Tests
New categories/events are introduced: