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

Upgrade protobuf to 4.21.x #49168

Merged
merged 47 commits into from
Feb 13, 2023
Merged

Upgrade protobuf to 4.21.x #49168

merged 47 commits into from
Feb 13, 2023

Conversation

risemeup1
Copy link
Contributor

@risemeup1 risemeup1 commented Dec 19, 2022

PR types

others

PR changes

others

Describe

升级Protobuf,在python/requirements中去掉protobuf版本限制

目前除了Windows下没有将protobuf版本升级高最高的v21.12之外,Paddle主框架和其它套件均完成升级,为了不影响其他人的工作,暂时先不升级windows下的protobuf版本,等windows下编译问题解决之后再同步windows的protobuf版本升级
image

@paddle-bot
Copy link

paddle-bot bot commented Dec 19, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@paddle-bot
Copy link

paddle-bot bot commented Dec 19, 2022

✅ This PR's description meets the template requirements!
Please wait for other CI results.

@risemeup1 risemeup1 force-pushed the protobuf branch 2 times, most recently from b512d98 to 74fe2ed Compare December 23, 2022 11:25
tianshuo78520a
tianshuo78520a previously approved these changes Jan 12, 2023
tianshuo78520a
tianshuo78520a previously approved these changes Feb 8, 2023
tianshuo78520a
tianshuo78520a previously approved these changes Feb 9, 2023
tianshuo78520a
tianshuo78520a previously approved these changes Feb 9, 2023
atomicMin(
reinterpret_cast<unsigned long long int*>(&key_index[pos]), // NOLINT
static_cast<unsigned long long int>(index)); // NOLINT
atomicMin(reinterpret_cast<unsigned int*>(&key_index[pos]), // NOLINT
Copy link
Contributor

Choose a reason for hiding this comment

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

why need to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

多谢review,这个之前实验的时候改的数据类型,忘了改回来,下个PR把这个改回来。

COMMAND ${PROTOBUF_PROTOC_EXECUTABLE} -I${CMAKE_CURRENT_SOURCE_DIR}
--cpp_out "${CMAKE_CURRENT_BINARY_DIR}" ${ABS_FIL}
COMMAND ${PROTOBUF_PROTOC_EXECUTABLE} -I${PADDLE_SOURCE_DIR} --cpp_out
"${PADDLE_BINARY_DIR}" ${ABS_FIL}
Copy link
Contributor

Choose a reason for hiding this comment

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

why change dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

protoc编译的时候,推荐使用根目录,然后以绝对路径import文件

Copy link
Contributor

@zhangbo9674 zhangbo9674 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@zhwesky2010 zhwesky2010 left a comment

Choose a reason for hiding this comment

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

LGTM

elseif(WITH_IPU)
set(PROTOBUF_REPOSITORY ${GIT_URL}/protocolbuffers/protobuf.git)
set(PROTOBUF_TAG d750fbf648256c7c631f51ffdbf67d7c18b0114e)
Copy link
Contributor

Choose a reason for hiding this comment

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

ASCEND和IPU这些新硬件设备应该不用改tag吧?不过新硬件设备上编译时可能没有走这些分支。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

应该不用

elseif(WITH_IPU)
set(PROTOBUF_REPOSITORY ${GIT_URL}/protocolbuffers/protobuf.git)
set(PROTOBUF_TAG d750fbf648256c7c631f51ffdbf67d7c18b0114e)
set(PROTOBUF_TAG v21.12)
elseif(WIN32)
set(PROTOBUF_REPOSITORY ${GIT_URL}/protocolbuffers/protobuf.git)
# Change the tag to support building with vs2019
set(PROTOBUF_TAG 01a05a53f40ca2ac5f0af10c6cc0810bee39b792)
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows目前没有升级protobuf版本,后续有计划升级吗?对windows上支持gcc12有影响吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

后续有升级计划,威哥在跟进,对windows上gcc12是否有影响我要验证一下

Copy link

Choose a reason for hiding this comment

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

paddle window 下 protobuf版本过低已经和其他依赖发生冲突了,这块是否有新进度?
如果需要,如何支持?

else()
set(PROTOBUF_VERSION 3.1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

不重要,这里windows上的PROTOBUF_VERSION不是21.12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不是,还是之前的低版本protobuf_version

Copy link
Contributor

Choose a reason for hiding this comment

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

是否需要为windows加一个PROTOBUF_VERSION的判断逻辑?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok,我下一个PR加一下,这样更好

@risemeup1 risemeup1 merged commit 15d9339 into PaddlePaddle:develop Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants