integrated pybinding into fast-downward#1
integrated pybinding into fast-downward#1Emanuele-Tirendi wants to merge 4 commits intoroeger:pybind11-playgroundfrom
Conversation
FlorianPommerening
left a comment
FlorianPommerening
left a comment
There was a problem hiding this comment.
This is going in the right direction but the Python interface should be part of the search code and the CMake files for the library and the search code should be merged.
| so if in doubt, use that one. | ||
|
|
||
| Run with one of the following two commands | ||
| $ ./fast-downward.py pybindings-command-line |
There was a problem hiding this comment.
I don't think we need to integrate the call into the driver for now. You can of course do this for the prototype if it helps to test things but for now, the goal is just to compile a library.
README_PYBIND.txt
Outdated
| Notes on the integration into fast-downward | ||
|
|
||
| * The command "make install" still needs to be done manually. | ||
| This could be integrated into build.py. |
There was a problem hiding this comment.
I think make parameters can be passed to build.py as well, but the installation of the library also is not the most important thing right now.
README_PYBIND.txt
Outdated
| * The command "make install" still needs to be done manually. | ||
| This could be integrated into build.py. | ||
| * The path where the library is installed is now hard coded in | ||
| src/pybindings/CMakeLists.txt. This could be integrated into |
There was a problem hiding this comment.
We'll have to think about what a good way of installing the library is. Did you find any "best-practice" guide on this?
There was a problem hiding this comment.
No, I didn't. For now I took the install command away and let it only be done in the builds/python_library/bin folder and I added a comment whether it would be preferable to put it in builds/python_library/pybind_libraries. You'll see it after I push my commits again.
| import logging | ||
| import os | ||
| import sys | ||
| import importlib |
There was a problem hiding this comment.
Eventually, the goal is to just do import pydownward here. But this is definitely a later step. For now the goal is to generate the library file and expose the right classes in the right way.
src/CMakeLists.txt
Outdated
| if (NOT PYBINDING_LIBRARY) | ||
| add_subdirectory(search) | ||
| else() | ||
| add_subdirectory(pybindings) |
There was a problem hiding this comment.
The pybindings directory was just the prototype and should eventually go away. the source code should be inside search which means we have to merge the cmake files of pybindings and search.
src/pybindings/CMakeLists.txt
Outdated
| set(CMAKE_CXX_STANDARD_REQUIRED ON) | ||
| set(CMAKE_CXX_EXTENSIONS OFF) | ||
|
|
||
| # TODO: Can you use the same flags here as in search/CMakeLists.txt? |
There was a problem hiding this comment.
We should keep all the setup of flags in the general part, so it should not be needed here.
src/pybindings/CMakeLists.txt
Outdated
| include_directories("${CMAKE_CURRENT_SOURCE_DIR}/../search/") | ||
|
|
||
| # TODO: Need to integrate DownwardFiles.cmake | ||
| file (GLOB SOURCE_FILES "../search/*.cc" "../search/*/*.cc") |
There was a problem hiding this comment.
GLOB is a hack that we should avoid. The file src/search/DownwardFiles.cmake defines the list of sources and search/CMakeLists.txt already collects them and handles the dependencies.
There was a problem hiding this comment.
We could maybe treat the file that defines the interface (currently called test.cc but we have to find a better name) as a CMake plugin. Then the build configuration for the library would enable this plugin and all other configs would disable it.
src/pybindings/CMakeLists.txt
Outdated
|
|
||
| ## == Install == | ||
|
|
||
| set(PYTHON_LIBRARY_DIR ${CMAKE_CURRENT_SOURCE_DIR}/../../trypybindings) |
There was a problem hiding this comment.
trypybindings also is a temporary thing and should not be hard-coded here. For now, I'd ignore the installation step.
trypybindings/output.sas
Outdated
| @@ -0,0 +1,71 @@ | |||
| begin_version | |||
There was a problem hiding this comment.
Having this file in the repo for the prototype is fine but of course in the long runs, we should remove it again.
trypybindings/output.sas
Outdated
| 1 | ||
| 0 0 0 1 | ||
| 1 | ||
| end_operator |
There was a problem hiding this comment.
I cannot comment on the next file (the compiled python library) but it should not be checked in. It will be different in every build and for every user. You can add it to .gitignore
2 participants
No description provided.