-
Notifications
You must be signed in to change notification settings - Fork 863
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
Micro batching example #2210
Micro batching example #2210
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2210 +/- ##
==========================================
+ Coverage 69.44% 70.69% +1.24%
==========================================
Files 77 78 +1
Lines 3450 3638 +188
Branches 57 57
==========================================
+ Hits 2396 2572 +176
- Misses 1051 1063 +12
Partials 3 3
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 @mreso , LGTM.
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.
many users ignore TS good examples. Micro batching is a good feature to boost TPS. We need promote it to a feature announcement, not just an example.
can we move extract the core parts such as micro_batching.py and micro_batch_handler.py to handler utils so that users can easily plugged in customer handler?
def initialize(self, ctx): | ||
super().initialize(ctx) | ||
|
||
config_file = Path(ctx.system_properties["model_dir"], "micro_batching.json") |
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.
can we use the new feature model-config.yaml to set these parameters?
all of the parameters can be accessed via ctx. model_yaml_config
self.handle.parallelism = config["parallelism"] | ||
self.handle.micro_batch_size = config["micro_batch_size"] |
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.
users may be thought "parallelism" and "micro_batch_size" are related to large model parallel. Can we have a good naming to differentiate?
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.
Sure, can just call them mb_size and mb_parallelism
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.
Went with ["micro_batching"]["parallelism"], that should be sufficient for users to differentiate.
00b3bb5
to
9d06fe6
Compare
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.
ts/handler_utils is handler utils dir. could you move utils/micro_batching.py to ts/handler_utils/microbatching?
ts/handler_utils/__init__.py
Outdated
from .micro_batching import MicroBatching | ||
|
||
__all__ = ["MicroBatching"] |
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.
this needs to be removed.
e6b97c1
to
b3b487a
Compare
@mreso I see the MacOS CI is failing because of Resource issue. Can you please check if its because of this PR? |
Description
This PR introduces a new example demonstrating micro batching for a ResNet image classifier.
Type of change
Please delete options that are not relevant.
Feature/Issue validation/testing
Please describe the Unit or Integration tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.
Checklist: