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

Refactoring: add helper class to bind qnn tensor -> ggml tensor #2

Open
wants to merge 17 commits into
base: qualcomm_qnn_backend_for_ggml
Choose a base branch
from

Conversation

chraac
Copy link

@chraac chraac commented Jun 17, 2024

  • Self Reported Review Complexity:
    • Review Complexity : Low
    • Review Complexity : Medium
    • Review Complexity : High
  • I have read the contributing guidelines

As I said in your upstream PR, better to have a function for wrapping ggml_tensor into Qnn_Tensor_t.
So here i create a PR for it.

Run test on cpu backend, works well
5338c775ff17bc845aca02c6380446e

Run on npu backend, also works well:
1648ce8fa1b30cb385978edd4840dbd

ggml-qnn.cpp Outdated
QNN_LOG_WARN("alloc rpcmem failure, %s\n", strerror(errno));
QNN_LOG_DEBUG("tensor%p name %s", _qnn_tensor, QNN_TENSOR_GET_NAME(*_qnn_tensor));
_context = nullptr;
// TODO: should we free the tensor here?
Copy link
Author

Choose a reason for hiding this comment

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

Should we free the _qnn_tensor here create by tensorCreateGraphTensor (line 1979)?

Copy link
Owner

@zhouwg zhouwg Jun 17, 2024

Choose a reason for hiding this comment

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

不需要。高通QNN SDK貌似没有提供类似的函数。看高通的文档,貌似SDK内部会管理这些内部资源。

Copy link
Owner

Choose a reason for hiding this comment

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

您在这个PR里提到的问题我此前已经注意到了,暂时没有理解为啥会这样。高通QNN SDK的技术资料比较少,目前只有哪个SDK reference manual.

Copy link
Author

Choose a reason for hiding this comment

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

个人判断应该是漏了些同步操作,不过确实没啥信息

Copy link
Owner

@zhouwg zhouwg Jun 17, 2024

Choose a reason for hiding this comment

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

不太清楚,我做过各种实验。没有查到公开的有价值的参考资料。

从已有的公开资料来看,国内目前已经实现了高通NPU加速的公司有几家,其中发布了Open MiniCPM-V的面壁智能是其中一家。如果您是商业公司雇员,可以联系对方。如果是我这样的独立开发者,在没有与QTI签定NDA以及得到高通技术支持的情况下,试图完全做出来,难度可能不小:比如哪些出错信息代码,根本不知道具体是啥意思。

Copy link
Author

Choose a reason for hiding this comment

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

之前用过Qualcomm的GPU profiler,也是bug一堆,这种问题感觉得等他们自己修了,我们要workaround的话,会花掉很多无谓的时间

Copy link
Owner

@zhouwg zhouwg Jun 17, 2024

Choose a reason for hiding this comment

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

赞同您的观点。最好得到高通的技术支持。

@zhouwg
Copy link
Owner

zhouwg commented Jun 17, 2024

thanks for your PR. we can discuss this problem in my personal learning&study project:https://github.com/zhouwg/kantv/tree/ggml-qnn-refine/core/ggml/llamacpp/tests/ggml-qnn.

不了解您是啥背景?类似我这样闲着没事干的独立个人开发者?还是AI相关公司雇员?如果您是公司雇员,可以联系面壁智能(他们应该已经做到了高通NPU的加速).

没想到您还对这个问题感兴趣。我现在对上游项目不感兴趣了,如您愿意,可以在我的学习&研究项目里研究ggml-qnn相关问题,可以用中文,这样更方便,最后将研究结果贡献给社区。不用费劲提交给上游了(事实上,自从哪个ggml-rpc.cpp相关的多个pr被合并到master分支后------很失望)。

@chraac
Copy link
Author

chraac commented Jun 17, 2024

thanks for your PR. we can discuss this problem in my personal learning&study project:https://github.com/zhouwg/kantv/tree/ggml-qnn-refine/core/ggml/llamacpp/tests/ggml-qnn.

不了解您是啥背景?类似我这样闲着没事干的独立个人开发者?还是AI相关公司雇员?如果您是公司雇员,可以联系面壁智能(他们应该已经做到了高通NPU的加速).

没想到您还对这个问题感兴趣。我现在对上游项目不感兴趣了,如您愿意,可以在我的学习&研究项目里研究ggml-qnn相关问题,可以用中文,这样更方便,最后将研究结果贡献给社区。不用费劲提交给上游了(事实上,自从哪个ggml-rpc.cpp相关的多个pr被合并到master分支后------我很失望,上游项目已经进入垃圾时间了,没啥核心改进)。

我个人对这个问题还是比较乐观,毕竟rpc那系列pr我也看过,并且日常我也用,和qnn这个,我认为也不冲突。
个人对llama.cpp添加qnn backend还是看好的,另外这个PR确实也大,所以review比较慢,还是请你别灰心,加油!要是有机会的话还是尽量merge到upstream,毕竟这样可以少很多事。

我做这个还是出于个人爱好,主要可以学习交流新东西,兴趣使然。所以也没打算和厂商联系,基于公开资料做做这样。

@chraac
Copy link
Author

chraac commented Jun 17, 2024

  • 这个可能我还是说两句我的经验,我刚开始提PR也是这样,觉得PR的某些评论有点针对个人,后来打交道多了也就想开了,换个位置思考,其实大家都是基于兴趣爱好做点开源的东西,也不是工作,review的人可能也只是兴趣爱好,基于这个出发点,是不是大家合作就会好些。
  • PR review时间长的问题,我也经常遇到(我以前有过经验,一个大PR拖了几个月),这个可能确实无解,因为大家都是业余玩这个,时间也不固定,我一般会把我的思路写下来,然后每部分代码加点注释啥的,这样review也容易些。
  • PR优先级这个确实无解,有可能某些PR就是很重要,但是社区没有足够的有相关背景的人来review,这种情况确实存在,看起来您的PR就是处于类似的位置,所以对其他reviewer可能难度很大,这个还得请老哥别灰心。
  • 一个社区确实是会有不同的声音的,确实会有部分人反对,也是一种声音,但是社区内部还是比较民主的,经过这些年,我开始觉得有个反对的声音挺重要的
  • 另外,老哥,不必妄自菲薄,你能业余时间做些开源贡献,其实已经比很多人厉害了;另外,每天对着代码对着comment,确实会容易有情绪,这种情况确实存在,也只能说尽量控制,不影响判断,也尽量不泄露到其他地方(这个真难,我也做不到,唉),这样反倒容易让不相干的人卷进来。
  • 还有就是语言的问题,其实我们自己都玩llm,现在llm已经能很好的处理语言之间转换的问题了,其实用好了,可以减少很多语言障碍。

感谢老哥回复,你用业余时间做到这样,已经不错了,加油!

@chraac chraac force-pushed the dev-function-to-map-tensor branch from 4d70039 to 65a14d9 Compare June 18, 2024 15:09
@chraac
Copy link
Author

chraac commented Jun 19, 2024

@zhouwg 老哥,有空麻烦看看PR,如果没啥问题能不能帮忙merge到你的分支去。
这个backend还是有人关注的,请不要放弃。
我也会抽点时间继续完善这个分支。
最后再次感谢老哥的努力!

@chraac chraac requested a review from zhouwg June 19, 2024 02:48
@chraac chraac force-pushed the dev-function-to-map-tensor branch from 7a77028 to dfe159f Compare June 19, 2024 03:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants