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

Expose cloud and edge inference speed metrics #173

Merged
merged 3 commits into from
Feb 23, 2024
Merged

Conversation

cat101
Copy link
Member

@cat101 cat101 commented Feb 6, 2024

This change helps users get a breakdown of the time spent during inference. This is useful to experiment with different inference pipelines.

Copy link
Member

@camiloaz camiloaz left a comment

Choose a reason for hiding this comment

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

As a general comment, in the past, we have been asked by @bibireata and other people to remove these performance metrics from the response of cloud inference. I believe we should not expose this for cloud inference, maybe.

Comment on lines 33 to 35
# performance_metrics keeps performance metrics for the last call to _do_inference()
performance_metrics: Dict[str, int] = {}

Copy link
Member

Choose a reason for hiding this comment

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

Why store it as a global variable? Why not associate it with a Predictor instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess my brain was running low on creativity 😞 ... I changed to a private variable to the class...thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the comment about performance metrics I think in the long term we need a tutorial on how to optimize models for speed and how to use metrics to profile and find problems. For now they are a bit obscure but at least now we expose them as part of the predict class 🤷‍♂️

Copy link
Member

@pappacena pappacena left a comment

Choose a reason for hiding this comment

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

LGTM

@cat101 cat101 merged commit 5d6a85a into main Feb 23, 2024
13 checks passed
@cat101 cat101 deleted the matias-expose-metrics branch February 23, 2024 20:34
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.

3 participants