-
Notifications
You must be signed in to change notification settings - Fork 30
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
(shortfin-sd) Adds tooling for performance measurement. #380
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest upgrading that decorator to take kwargs telling it what to vs sniffing qualname
ret = await fn(*args, **kwargs) | ||
duration_str = get_duration_str(start) | ||
logger.info(f"Completed {fn.__qualname__} in {duration_str}") | ||
if fn.__qualname__ == "ClientGenerateBatchProcess.run": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on with this? Seems like you should just have a kwarg on the decorator to tell it what to do. There's a standard (but tricky) idiom for that using functools.partial...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pointer -- I didn't end up using partial here but adapted to take a few kwargs. There's still some yuck there where we ping attributes of the decorated method's class to figure out batch size, but still more flexible than before. I may revisit later.
Output example: