-
Notifications
You must be signed in to change notification settings - Fork 75
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
[Wait for 2759][GPU/OpenCL] Initial version of LM Head layer with OpenCl ops and updated Addition Layer with latest pipeline changes. @open sesame 12/02 16:21 #2752
base: main
Are you sure you want to change the base?
Conversation
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2752. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/. |
cibot: @yashSingh0723, nntrainer/layers/cl_layers/lm_head_layer_cl.h does not include Doxygen tags such as @file @brief @author @bug. You must include the Doxygen tags in the source code. Please refer to a Doxygen manual at http://github.com/nnstreamer/TAOS-CI/blob/main/ci/doc/doxygen-documentation.md |
781e16e
to
8bd85e8
Compare
cibot: @yashSingh0723, nntrainer/layers/cl_layers/lm_head_layer_cl.h does not include Doxygen tags such as @file @brief @author @bug. You must include the Doxygen tags in the source code. Please refer to a Doxygen manual at http://github.com/nnstreamer/TAOS-CI/blob/main/ci/doc/doxygen-documentation.md |
8bd85e8
to
210d217
Compare
cibot: @yashSingh0723, nntrainer/layers/cl_layers/lm_head_layer_cl.h does not include Doxygen tags such as @file @brief @author @bug. You must include the Doxygen tags in the source code. Please refer to a Doxygen manual at http://github.com/nnstreamer/TAOS-CI/blob/main/ci/doc/doxygen-documentation.md |
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.
@yashSingh0723, 💯 All CI checkers are successfully verified. Thanks.
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 glad to see the implementation of the key blocks for LLM in opencl version.
Here are some opinions in my side:
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.
@yashSingh0723, 💯 All CI checkers are successfully verified. Thanks.
27cf3d5
to
0b9ff2a
Compare
cibot: @yashSingh0723, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2752-202411221838150.57073402404785-0b9ff2aa74fcfcdb01e1dff03a229df8f4457b55/. |
0b9ff2a
to
57ae5ed
Compare
cibot: @yashSingh0723, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2752-202411222045320.65086197853088-57ae5ed2a75efcba64c20bad9b312233da9d6a95/. |
57ae5ed
to
5c0e60a
Compare
cibot: @yashSingh0723, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2752-202411251537140.93050694465637-5c0e60a0a35fdc17a75ad5e5420b9086781d56df/. |
5c0e60a
to
d0b5e1c
Compare
cibot: @yashSingh0723, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2752-202412021954220.7212700843811-d0b5e1c22ba9c2426ecf23fb18344942573209b0/. |
d0b5e1c
to
2d8c2e8
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.
@yashSingh0723, 💯 All CI checkers are successfully verified. Thanks.
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.
LGTM
@@ -108,7 +108,8 @@ enum LayerType { | |||
LAYER_UPSAMPLE2D, /**< Upsample 2D Layer type */ | |||
LAYER_RMSNORM = ML_TRAIN_LAYER_TYPE_RMSNORM, /**<RMS NORM Layer */ | |||
LAYER_TRANSPOSE = ML_TRAIN_LAYER_TYPE_TRANSPOSE, /**< Transpose Layer type */ | |||
LAYER_UNKNOWN = ML_TRAIN_LAYER_TYPE_UNKNOWN /**< Unknown */ | |||
LAYER_UNKNOWN = ML_TRAIN_LAYER_TYPE_UNKNOWN, /**< Unknown */ | |||
LAYER_LM_HEAD = ML_TRAIN_LAYER_TYPE_LM_HEAD, /**< LM Head Layer */ |
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 is not a big deal, but I think setting the LAYER_UNKNOWN at the end would be better.
/** | ||
* @copydoc bool supportBackwarding() const | ||
*/ | ||
bool supportBackwarding() const override { return true; }; |
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.
bool supportBackwarding() const override { return true; }; | |
bool supportBackwarding() const override { return false; }; |
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.
Updated in the latest commit, Thank you for pointing it out.
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.
LGTM
… and Update Addition Layer on GPU with latest Pipeline changes Added initial version of LM head layer fpr GPU and removed dependencies of cl_context for addition_layer. Signed-off-by: Yash Singh <[email protected]>
Clang format fixed and comments removed. Rebased PR with latest changes. Signed-off-by: Yash Singh <[email protected]>
Addressed multiple review comments Removed unnecessary code Added nnlayer golden files Signed-off-by: Yash Singh <[email protected]>
2d8c2e8
to
0c9e7aa
Compare
Reviewed PR comments SupportBackwarding() set to false Signed-off-by: Yash Singh <[email protected]>
Added initial version of LM Head Layer for GPU. Also Updated the addition layer for GPU with latest pipeline changes.
Changes added with this PR:
lm_head_layer_cl.cpp
added containing the newCustomLMHeadLayerCl
class for OpenCL implementation.unittest_layers_lm_head_cl.cpp
to test LM Head Layer on GPU.cl_context
dependencies for addition layerSigned-off-by: Yash Singh [email protected]