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

[onert] Suggestion to modify unnecessary code according to pybind11 update #13115

Closed
jaemaning opened this issue Jun 5, 2024 · 5 comments
Closed

Comments

@jaemaning
Copy link
Contributor

Background

Recently, I discovered that a long-standing bug in pybind11 has been fixed. This fix eliminates the need to directly import Python development components (libs, headers) from various Linux environments.

Reference to the current implementation:

# CMakeLists.txt
# refer to https://github.com/Samsung/ONE/issues/11368
# Tell pybind11 where the target python installation is
#
# FindPythonLibs is deprecated since 3.12, and recommand to use FindPython.
# But on cross compile, FindPython is not working for target environment
# For workaround, use PythonLibs
find_package(PythonLibs REQUIRED)
set(PYTHON_MODULE_PREFIX "lib" CACHE INTERNAL "Cross python lib prefix")
set(PYTHON_MODULE_EXTENSION ".so" CACHE INTERNAL "Cross python lib extension")
# Disable pybind11 python search mechanism
set(PYTHONLIBS_FOUND TRUE CACHE INTERNAL "")
# Install pybind11
nnfw_find_package(Pybind11 REQUIRED)
if(NOT Pybind11_FOUND)
message(STATUS "Build onert/python: FAILED (Pybind11 is missing)")
return()
endif()

Proposed Change

Given this fix, I suggest updating the code to the following:

# Install pybind11
nnfw_find_package(Pybind11 REQUIRED)
if(NOT Pybind11_FOUND)
  message(STATUS "Build onert/python: FAILED (Pybind11 is missing)")
  return()
endif()

Opinion

I would like to know your thoughts on this proposed change.

Reference

For more details on the bug fix, please see: pybind/pybind11#4805

@hseok-oh
Copy link
Contributor

hseok-oh commented Jun 5, 2024

Thanks your suggestion. Then we need to use more latest pybind11 version, right? (not currently using v2.11.1)

@jaemaning
Copy link
Contributor Author

I understand that ONE is currently using pybind v2.11.1.

envoption(EXTERNAL_DOWNLOAD_SERVER "https://github.com")
envoption(PYBIND11_URL ${EXTERNAL_DOWNLOAD_SERVER}/pybind/pybind11/archive/v2.11.1.tar.gz)

And the above application requires the use of pybind 2.12.0 releasing version.

Pybind11 Version 2.12.0 - bug fixes List

image

If it is okay to use the currently released version of pybind11 v2.12.0, I will test it.

Reference

https://github.com/pybind/pybind11/releases

@hseok-oh
Copy link
Contributor

hseok-oh commented Jun 12, 2024

Based on release note, this feature works on cmake 3.18+.
Since we are using cmake 3.16.3 (ubuntu 20.04's default cmake version), we cannot use this feature.

@jaemaning
Copy link
Contributor Author

oh, I didn't check the cmake version
Thanks for checking this

@hseok-oh
Copy link
Contributor

@jaemaning Thank you for inform us about this. When we ready to upgrade cmake version, we can use this feature.

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

No branches or pull requests

2 participants