Road map for migrating to PyBind11 and Python3.x support.

[dkeeney]

Here is what I propose:
I am going to abandon issue #74 (Replace APR) and PR #79. I was trying to do this in too large of a chunk. I also messed up and ended up with massive whitespace change due to line ending changes (working on windows trying to get MinGW to work). lets break up the effort into smaller parts.

1. New PR to replace existing header only Boost with Boost with filesystem and system modules.

2. A new PR for each file, or very small groups of files, to replace the APR calls to std:: or boost:: calls.
   This will be a lot of PR's so I will need help in getting these reviewed. The work is already done, just need to open the PR and copy my changes on a file by file bases.

3. A new PR to remove the APR libraries...delete only

Then we can look at removing SWIG and replacing it with PyBind11.
4) Change some coupling in RegionImplFactory so that the Python related code can be isolated.

5. Build PyBind11 interface module (based on work by @chhenning). Might not be able to run both PyBind11 and SWIG in the same module but maybe we could just disable SWIG for this PR.

6. Remove SWIG.

At this point we can look at Porting more modules from Python code into the core such that it can be used as a stand-alone C++ library. Specifically SPRegion and TMRegion as well as lots of encoders.

• There is a major problem with using the version of MinGW that we are currently using. It is a special version hacked up to work with Python 2.7 and SWIG which is no longer supported. I propose that we not support that platform during the transition, working only with Linux and OSx platforms. Then after step 6 we add Visual Studio 2017 platform and perhaps re-introduce the real MinGW platform.

[breznak]

Thanks for rethinking this, David! And great to see you back in full force! 👍

I've just returned from an internship, so I'm catching up with all the news here.

• can I help you somehow with the process of reusing Replace apr (Abandoned) #79 in smaller pieces?
• agree with the Windows/MingGW problem, will just open an issue to document it and we can break it! (hopefully temporarily)

[breznak]

to replace the APR calls to std:: or boost:: calls

We've also discussed utilising c++17 (namely TS Filesystem & TS Parallel) to also avoid boost at all, although there were some (build flags) problems getting it work. Also compiler support is on edge. What c++ version do we settle at? I'd suggest 17, even though new, as before this repo matures, the support should be more common-place (?)

This will be a lot of PR's so I will need help in getting these reviewed

Just tag me and I'll do the review ASAP

Remove SWIG.
At this point we can look at...

At that point we should reintroduce the Windows CI too, #69

[dkeeney]

I think it best to stay with C++11 until APR is removed. Then replace any remaining parts of Boost with std::. There are some things that don't have a direct std:: replacement...Graphic Algorithms in particular. So lets take it in smaller steps.

[breznak]

I'll comment/review on your summary from PR #79

1. Removed APR, apr-iconv, apr-util and yaml from external/common/share
   yaml was not being used. Keeping Yaml-cpp which is being used.
   external/CMakeLists.txt
   external/Apr1Lib.cmake (deleted)
   external/AprUtil1.Lib.cmake (deleted)
   external/commong/share/apr, apr-iconv, apr-util,yaml (deleted)

we could do this now.

2. Removed boost include files and added a full boost install
   so we can get the system and filesystem libraries as well as the include files.
   external/common/share/boost/*.hpp (removed)
   Boost.cmake (new)
   external/CMakeLists.txt

Done (with changes), Boost is in master, linking to it fails. We'll need to fix that first.

3. implemented a superBuild with all of external dependancies
   as one external project and nupic core library as the second which depends on the first.
   The dependancies are found using find_package() rather than having the references passed.
   CMakeLists.txt
   src/CMakeLists.txt
   external/CMakeLists.txt
   Swig.cmake
   Boost.cmake
   YamlCppLib.cmake
   CommonCompilerConfig.cmake (CPP standard is passed in)
   CombineUnitxArchives.cmake (trying to get MinGW to work)
   NupicLibraryUtils.cmake

avoid, if possible

4. Replaced APR file handling with boost::filesystem or std::filesystem
   Directory.cpp/hpp
   Path.cpp
   OS.cpp
   PathTest.cpp
   DirectoryTest.cpp

I'd use either, or, but not both. Decide for one approach and we'll stick with it.
Either way, in the code let's make it future-proof:
Instead of boost::some_fs_function()
use:

using boost::some_fs_function;

...code
   a = some_fs_function();

That way we'll be able to later swith with just a few line-changes.

5 Replaced APR functions with C++11 std:: functions
NuPIC.cpp
Timer.cpp/hpp
TimerTest.cpp
Env.cpp/hpp
FStream.cpp
OS.cpp/hpp
OSUnix.cpp
OSWin.cpp
UnitTestMain.cpp

👍

6. Replaced boost::circular_buffer with std::deque
   Link.cpp/hpp
   RegionImplFactory.cpp

👍

7. Replaced boost::shared_ptr with std::shared_ptr
   When I started I thought this would be C++17 so I was
   removing all references to boost. When we fell back to
   C++11 these still worked so I retained those changes.
   Not all boost references were removed.
   RegionImplFactory.cpp
   TestNode.cpp
   UniformLinkPolicy.cpp/hpp
   YamlUtils.cpp
   Buffer.cpp/hpp
   Value.cpp/hpp -- also the string value did not need shared_ptr.
   PyRegion.cpp
   YAMLUtilsTest.cpp
   ArrayTest.cpp
   ValueTest.cpp

we can switch to, at least c++14, where shared_ptr is, imho

8. Removed boost::concept_check
   Math.hpp

👍

9. Replaced boost::unordered_set with std::unordered_set
   SparseMatrix.hpp

👍

10. Changes in Stlio.hpp and Utils.hpp ended up being only formatting.
    Spend a lot of time messing with Functors and ended up without substantive changes.

11. Removed the NTA_ types; NTA_Int32 to Int32, etc.
    Buffer.cpp/hpp
12. Replaced boost::type_index for std::type_index
    NumpyVector.hpp
13. Added a trim() function (needed for Path.cpp) and replaced some APR functions.
    StringUtils.cpp/hpp

14. added f and u to numeric constants
    These were to remove compiler warnings.
    ConnectionsTest.cpp
    SDRClassifierTest.cpp
    SegmentTest.cpp
    SpatialPoolerTest.cpp
    TemporalMemoryTest.cpp
    YAMLUtilsTest.cpp
    SegmentMatrixAdapterTest.cpp
    TopologyTest.cpp
    ExceptionTest.cpp
    RandomTest.cpp

ok, if needed

15. replaced ASSERT_EQ(false, bool value) with ASSERT_FALSE(bool value)
    The MinGC compiler did not like it.
    SDRClassifierTest.cpp
    ScalarTest.cpp

omit this, as we don't plan MinGW for the future?

16. Added more testing for Array. (actually something that got left out of previous PR)
    and some fixes to exception handling.
    ArrayTest.cpp

that's great

17 Formatting only
Cell.cpp
Cells4.cpp/hpp

skip formatting changes? Unless it's some nice cleanup of the code

[dkeeney]

Well, maybe rather than doing this a file per PR, I could do PR's in these 15 steps.

• Step 1 would need to go after step 5.
• Step 2 is done
• Not do step 3
• Step 14 is to suppress warnings in Windows, could be later
• Step 15 we probably don't need to do.
• Sounds like 17 is already done.

[breznak]

ather than doing this a file per PR, I could do PR's in these 15 steps.

yep, agree, just suggested that elsewhere.

Step 2 is done

Boost may need fixing. When actually using it in PR #79 , I got it to compile, but have link errors.

The ordering sounds good to me, then we can do the remaining steps.

[dkeeney]

As for C++11, C++14 or C++17

• shared_ptr is in C++11 which is most of the changes.
• About the only thing we could not do with C++11 was filesystem.
• Oh, and math/GraphicAlgorithms has stuff that only boost can do (I think)
  #include <boost/graph/adjacency_list.hpp>
  #include <boost/graph/bandwidth.hpp>
  #include <boost/graph/connected_components.hpp>
  #include <boost/graph/cuthill_mckee_ordering.hpp>
  #include <boost/graph/properties.hpp>

Otherwise it is just filesystem that is the problem.
Compiler support for filesystem and C++17: https://en.cppreference.com/w/cpp/compiler_support

• GCC 7.1 has <experimental/filesystem>, link with -libc++experimental or -lstdc++fs
• GCC 8 has link with -lstdc++fs
• GCC 9 expected to support filesystem
• Clang 4 (XCode10) has no support for filesystem, partial C++17
• Clang 7 has complete filesystem support for C++17
• Visual Studio 2017 15.7 (v19.14)supports filesystem with C++17
• MinGW has no support for filesystem.

[breznak]

Oh, and math/GraphicAlgorithms has stuff that only boost can do (I think)

Let's remove that. The class is not used anywhere in our codebase, even its doc says it's not used in production.

Clang 4 (XCode10) has no support for filesystem, partial C++17

Does it mean OSX cannot support c++17+fs? Or XCode is just an IDE and ppl could install clang7?

In my view the decision is: "c++11 + Boost" / "C++17 pure"
How is the platform support for c++17,fs?

• Linux (gcc8+ ok, gcc7 with help, gcc7 is pretty common-in Ubuntu LTS)
• OSX?
• Windows MSVS -OK, even on CI https://www.appveyor.com/updates/2018/11/09/

TL;DR: If OSX can get c++17+filesystem to work, I'd vote for pure c++17, and we can even get rid off Boost! Otherwise, go with c++17 & Boost for now.

[dkeeney]

• Removing GraphicAlgorithms: OK, will do.
• XCode10: The default compiler in OSx lags behind clang version. But yes, they could upgrade compiler. So it is the compiler version we need to key on.
• Problem is SWIG had some compile prob with C++17. Don't recall just what it was. But after we get rid of SWIG we could go pure C++17. That is the goal.

[dkeeney]

Talking about things to get rid of, here are some more candidates:

• os::FStream Does not do much as far as I can see.
• ntypes::Buffer There must be a better way to have a container of multiple data types (hold-over from Python) Suggest we replace it with a struct or class containing a field for each type and type id as selector. Maybe other things that would be better.

[breznak]

roblem is SWIG had some compile prob with C++17. Don't recall just what it was. But after we get rid of SWIG we could go pure C++17. That is the goal.

Ok, let's stick to it:

1. keep boost & c++11 (=now)
2. rid of swig (use pybind11)
3. c++17 + no boost

[breznak]

Thing is, do you want to deal with #115 now (smart pointers in Region etc), or proceed straight with 4,5,6 (pybind/swig) and leave pointers for later, as those are not necessarily important for that?

[breznak]

@dkeeney how much work for you will be pybind-ing SparseBinaryMatrix etc? As the issue SP-on-Connections #93 would investigate removal of the *Matrix classes.

[dkeeney]

I don't think SparseBinaryMatix would by much impact or be impacted by the PyBind11. So that could be done in parallel I would think.

I am tempted to finish the Region and Link pointers now but that could end up being a rabbit hole. Perhaps I should proceed to the PyBind part.

[breznak]

So that could be done in parallel I would think.

Great

the Region and Link pointers now but that could end up being a rabbit hole. Perhaps I should proceed to the PyBind part.

Entirely up to you, I'm lost in Engine,Regions,Links the same way you feel about Random :)

[dkeeney]

I have a small problem with one of the websites that I maintain and I need to go work on that but when that is finished I will start in on PyBind. I don't expect it to be more than a couple of days.

@chhenning has already done most of the work for PyBind so I will be just merging in his code mostly. This will be a large number of changes again. I was looking at this to see if there was some way to break it up but there does not seem to be much we could isolate. We could build the PyBind part in parallel with SWIG in one PR and then remove SWIG in a second. However there could be some conflict between the two if we tried to build both in one module. For one thing, how would we tell the Python code which interface to use. So lets just do the whole thing in one go.

I will need to go back and re-read the documentation on PyBind11 again so I can understand all of the py_xxx.cpp interface files. They don't look complicated but I am sure there will be complications once I start putting it together. Maybe @chhenning can help us out if we get stuck.

[dkeeney]

Ok, let me go take care of my website then I can get started on the PyBind.

…

On Sun, Nov 25, 2018 at 7:06 AM breznak ***@***.***> wrote: So that could be done in parallel I would think. Great the Region and Link pointers now but that could end up being a rabbit hole. Perhaps I should proceed to the PyBind part. Entirely up to you, I'm lost in Engine,Regions,Links the same way you feel about Random :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#81 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBa_37ezkyVNzQQU4chb72woXVB5KSKks5uyrHlgaJpZM4YO6XC> .

[breznak]

There's no hurry, finish your work.

This will be a large number of changes again. I was looking at this to see if there was some way to break it up but there does not seem to be much we could isolate

this is what I worry too. We've already disabled Win CI, if some deep problems occur, we'd be stuck with half-working repository.
Let's do this in one PR, but review per stages:

• add (and build) pybind (use latest stable ver, please)
• remove SWIG and all its hacks
• start implementing the interface

[dkeeney]

Ok, sounds good.
Oh, Pybind is include files only so there is no build. I will use the latest stable version.

[dkeeney]

I am starting to look at the PyBind11 PR. I am considering adding a new sub-directory 'py_interface' which is parallel with 'src'. This will contain all Python related code and a CMakeLists.txt. The top CMakeLists.txt would refer to this new CMakeList.txt with:

if (NUPIC_BUILD_PYEXT_MODULES)
  add_subdirectory(py_interface)
endif()

The structure would be something like:
py_interface
'--bindings (all of the binding code for pybind11)
'--plugin (source for Plugin components: PyRegionImpl.cpp/.hpp, RegisteredRegionImplPy.hpp, etc)
'--test

Any comment on this? Should I call them something else?

Where should we put the python3 code that was ported from the 'nupic' repository? Probably not here.

[dkeeney]

An alternative might be
py_interface
'--src
......'--bindings
......'--plugin
'--test

[breznak]

On Mon, 26 Nov 2018 at 16:31, David Keeney ***@***.***> wrote: I am starting to look at the PyBind11 PR. I am considering adding a new sub-directory 'py_interface' which is parallel with 'src'. This will contain all Python related code and a CMakeLists.txt. The top CMakeLists.txt would refer to this new CMakeList.txt with:

+1 for a separate dir for each language interface, with its cmake I would place that in src/nupic/bindings/xxx if (NUPIC_BUILD_PYEXT_MODULES)

add_subdirectory(py_interface) endif() The structure would be something like: py_interface '--bindings (all of the binding code for pybind11)

tthat means the pybind headers? If not, I’ll also add a local include/ dir

'--plugin (source for Plugin components: PyRegionImpl.cpp/.hpp, RegisteredRegionImplPy.hpp, etc) '--test Any comment on this? Should I call them something else? Where should we put the python3 code that was ported from the 'nupic' repository? Probably not here.

is it needed? If not, let’s just do bindings now. Later I’d drop python 2 and support only 3

[dkeeney]

What I was trying to do was isolate all of the Python components in their own directory which is not under src/CMakeList.txt but rather under their own parallel CMakeList.txt. I think of the src subdirectory as being the core C++ library component and would rather not put python stuff in there. But do you think they really should be there?

Repository
'--external   (External Builds for Yaml and PyBind11 downloads)
'--src     (everything needed for the nupic C++ library)
'--py_interface  (the python interface components)
'--sc_interface (the CSharp interface components)

Perhaps an another layout:

Repository
'--external   (External Builds for Yaml, Boost and PyBind11 downloads)
'--core    (everything needed for the nupic C++ library and tests)
'--python  (the python interface components and tests)
'--csharp (the CSharp interface components and tests)

What I think you are saying is something like this:

Repository
'--external
'--src
......'--nupic
..........'--xxx other library components
..........'--bindings
..............'--python
..............'--csharp

Is that really what you want?

Obviously the nupic .py code for Python3 (and perhaps Python2) would be someplace else. CSharp code that is a port of the python code would also go someplace else. Eventually we would have C++ components that are not part of the nupic C++ library; ports of the nupic .py code for running C++ only applications. Those also would go someplace else.

What I show as bindings in the previous message is manually written mappings for the interface (written in C++ but are similar in function to the python helper routines used with SWIG). This is not the PyBind11 header files. The header files are part of the dependency stuff. The plugin files are the parts of the Network framework that are required so that the Python interface can be a plugin.

[dkeeney]

Oh, and I was NOT going to build the python interface modules inside the nupic C++ statuc library. Each language interface would have their own static or dynamic libraries (or whatever they use) to connect in the language. Do you have a different vision?

[dkeeney]

@chhenning I am in the process of creating a CMakeLists.txt file that can build PyBind11. I was going to use your repository at https://github.com/chhenning/nupic as a template. But I don't see the .sln file. If it was in the 'build' folder, that folder is not uploaded. I would like to see that to see how you put together the binding libraries and installed them.

[chhenning]

Hi David, you can find the solution file here:

https://github.com/chhenning/nupic/tree/master/external/vs2017/nupic

[dkeeney]

Perfect. I must have looked right at it and somehow missed it.
Thanks.

[dkeeney]

@breznak I am hoping we can continue this discussion about the directory layout. We all need to be happy with how this is structured because it would be difficult to change later.

My arguments are

• the language interfaces need to be separate. A CSharp project should not need to have Python installed in order to build or run. Same with a C++ only project.
• The static core library is used by the language interfaces (nupic_core)
• The language interface code should not be contained in the core library. Avoids linking problems.
• The language interface dynamic libraries (such as nupic.python.algorithms.pyd, nupic.python.engine.pyd, nupic.python.math.pyd) call the core library and language libs (python.so, numpy.so).

So, my thinking is that the language interfaces should be parallel directories. They are dependent on the core but the core is not dependent on them.

Perhaps put all languages under a bindings directory. This might better convey what we are trying to do.

Repository
'--external   (External Builds for Yaml and PyBind11 downloads)
'--src  (everything needed for the nupic core  C++ library)
'--bindings
.... '--py  (the python interface components)
.... '--sc (the CSharp interface components)

Actually I like this arrangement better. What do you think?

[breznak]

Sorry @dkeeney , I posted my answer in the morning...but it actually didn't send.

Perhaps put all languages under a bindings directory. This might better convey what we are trying to do.

Repository
'--external   (External Builds for Yaml and PyBind11 downloads)
'--src  (everything needed for the nupic core  C++ library)
'--bindings
.... '--py  (the python interface components)
.... '--sc (the CSharp interface components)

Actually I like this arrangement better. What do you think?

I was just suggesting this 👍 It's a layout I find the most intuitive and modular.
Where each bindings/{lang: py, sc, cuda,...} would have its: external/ src/ tests/ doc/ etc.

I minor issue is with Pybind's location. Is it like SWIG and can offer interface to many languages? (then external), or only for PY (fine 👍 ), then maybe bindings/py/external/ ? My reasoning is that all the interface/module codebase would be self-contained.

[dkeeney]

PyBind is just for Python. So it should be in the py folder.
Ok, I will structure it up using the bindings folder.

Repository
'--external   (External Builds for Yaml and PyBind11 downloads)
'--src  (everything needed for the nupic core  C++ library)
'--bindings
.... '--py  (the python interface components)
.... '--sc (the CSharp interface components)

By the way, I am building this initially using MSVC so when I finish it should build under MSVC, linux, and OSx.

[breznak]

building this initially using MSVC so when I finish it should build under MSVC, linux, and OSx.

💯 That's pretty cool! So we'll be able to enable full Win support soon. #69

[breznak]

Another big step was #136 pybind11 !

• swig removed
• pybind added + it's interface .pyd files
• cmake modularized Modular repo structure #2
• py tests must be re-enabled
• Link c++ tests must be resolved Reenable Link tests from Pybind PR, add Link type conversions #140
• Resolve API issue with Spec.hpp and bool sparse Resolve sparse field in Spec #139

Couple of small things that might/not be done:

• put external/gtest files under tests/ , and move its cmake instractions from src/CMake to src/tests/CMake Is it a good idea?

[dkeeney]

@breznak you could go change the Spec structure in TestNode, lines 291 and 300 to populate the sparse flag to false (and perhaps move TestNode to test folder). Then the LinkTests could be turned back on. But this issue still needs to be resolved.

gtest is a Third party module and as such should be in external. It also should be downloaded and installed rather than having included files. It must be updated before we can use C++17.

[breznak]

How is the python generator step going?

[dkeeney]

Going quite well actually. Some minor complications.

• Isolating the python related calls to RegionImplFactory are completed.
• Some changes were made to interface function calls since @chhenning originally wrote the bindings so having to adjust. There is a bit of a learning curve on the syntax but I am getting the hang of it.
• The bindings assume Region pointers are replaced with shared_ptr. I am thinking it is a good idea. If the .py code should use a raw pointer after a Region has been removed, it would crash so this gives some protection. I coded up shared_ptr's for Regions before but had problems with SWIG. Now that SWIG is gone perhaps this will be easy this time. Same for Links.

[breznak]

With big fanfare! 🎉 🎉 👏 I'd like to thank @dkeeney , our now official "master of pulling large PRs and revamps successfully!" for finishing port from SWIG to PyBind! Based on @chhenning 's work, so big thank you to both! 👍

There are a few things in pybind migration that are TODO, but our migration is successfully over!
https://github.com/htm-community/nupic.cpp/milestone/6

This is now closed 💯 👍