Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
deepspeed mii fastgen example #2779
deepspeed mii fastgen example #2779
Changes from 9 commits
ae74521
d5221d1
618b06c
c85afb9
56d31d9
155a54a
1f790e3
42ee733
e7dbf45
8ccd897
648b0c9
6f741c1
c29cd6d
54db29c
d14c1ff
6d146eb
9a67dc2
18b4ce2
839782a
0d647ef
8f3d2d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 low level method should not be concerned about checking the LOCAL_RANK env environment. We should just not call it when its not "0".
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.
The checking is a central control to avoid this checking in handler implementation.
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.
I see the intention here. I still think this is not the preferred place to put this check. The function creating a response from some arguments should not need to know about the concept of ranks. In fact we already have a central point to check for the rank in this PR which is here.
The only point where this is missing is here. Looking at this I think we should move send_intermediate_predict_response out of ts.protocol.otf_message_handler (Users should not need deal with the modules containing our comms between fe/be. What if we drop otf protocol?). My first instinct is to move it under ts.handler_utils. What do you think?
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.
I'm fine to move it to handler_utils. The only concern is backward compatibility. It will break existing cx.
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.
AFAIK we do not guarantee bc as of now. Its probably time to plan out our bc strategy for future release as we're running out or 0.X version numbers. Anyways, if your concern is very high you can leave a wrapper in otf_message_handler that fires a deprecation warning and then calls the one in handler_utils until next release. cx will only get worse if we let the code rot.