Integration with nupic.core API

[david-ragazzi]

These changes allow that CMake builds nupic.core and links its output (libnupic.core.a) to nupic subprojects.

This depends of numenta/nupic.core-legacy#47 (at least depends of the SHA)

[rhyolight]

@david-ragazzi I just want to say thank you again for all your work. I'm very grateful! I can't wait to get this PR and the other checked in and running properly. You've done a great job. 🍻

[david-ragazzi]

@david-ragazzi I just want to say thank you again for all your work. I'm very grateful! I can't wait to get this PR and the other checked in and running properly. You've done a great job. 🍻

You're wellcome, @rhyolight !! As I always said, I believe this technology will change the world as we know.. I have in mind, that I am simply accelerating these changes when make the things easier to me and other developers*. In the end, I also grateful to you from Numenta to open-source Nupic and so I can use it in my projects..

PS: I've noted that more developers are contributing to the project (some in a shy way, but contribute anyway). This probally means that people are understanding better the code after we are using CMake!

[david-ragazzi]

Note that a missing job is "Repository Restructuring" using the default directories structure.. I prefered leave this job as "standby" until the integration being finally merged. On the contrary, existing PRs will lost the reference to location (we will need merge all this pendent PRs before pull the future "Repository Restructuring" PR)..

[rhyolight]

Something is off here. I'm getting the same error locally when tryign to build off both your PRs as Travis is getting:

CMake Error: The source directory "/Users/mtaylor/nta/nupic/nta/src" does not exist.
Specify --help for usage, or press the help button on the CMake GUI.
make: *** No targets specified and no makefile found.  Stop.
CMake Error at CMakeLists.txt:265 (message):
  Compiling 'nupic.core' library within /Users/mtaylor/nta/nupic failed.
Call Stack (most recent call first):
  CMakeLists.txt:707 (generate_submodule_library)

Still looking into it...

[rhyolight]

I think the problem is that you haven't updated the nupic.core submodule in your master branch. It is still pointing to https://github.com/numenta/nupic.core/tree/3ecc06a2e0b175a74381330d64a4ab803b71863d, when it should be pointing to https://github.com/david-ragazzi/nupic.core/tree/d76a786d35a3010e5314c4a8716743ff4e9604ba.

[david-ragazzi]

I think the problem is that you haven't updated the nupic.core submodule in your master branch. It is still pointing to https://github.com/numenta/nupic.core/tree/3ecc06a2e0b175a74381330d64a4ab803b71863d, when it should be pointing to https://github.com/david-ragazzi/nupic.core/tree/d76a786d35a3010e5314c4a8716743ff4e9604ba.

Is just change .gitmodules, right ?

[rhyolight]

You have to cd into the submodule directory and run a git command to update to the SHA you want. In my case, this was:

cd nta
git remote add david git@github.com:david-ragazzi/nupic.core.git
git fetch david
git checkout david/master

Previous HEAD position was 3ecc06a... Merge pull request #37 from breznak/use_c++98
HEAD is now at d76a786... Changed "Open 'Nupic._proj'" to "Open 'nupic.core._proj'"

cd ..
git status

Changes not staged for commit:
(use "git add ..." to update what will be committed)
(use "git checkout -- ..." to discard changes in working directory)

modified: nta (new commits)

git add nta

Now you can commit and push the submodule update.

[david-ragazzi]

Ok, when I go home I'll try this...

[david-ragazzi]

@rhyolight Now it works fine locally but still presents errors at Travis... :-(

[utensil]

@david-ragazzi This won't pass at Travis, reason as follows:

Local Environment

• david-ragazzi/nupic
• nta -> david-ragazzi/nupic-core@966a48f991318a4423656803dc083e66827b9385

Travis

• numenta/nupic plus pull/751/merge
• nta -> numenta/nupic.core

Tl;dr

Simply put, once #751 is merged, nta would refer to numenta/nupic.core , but there is no 966a48f991318a4423656803dc083e66827b9385 in numenta/nupic.core yet, because numenta/nupic.core-legacy#47 is not yet merged.

Details

git clone --depth=50 git://github.com/numenta/nupic.git numenta/nupic
git fetch origin +refs/pull/751/merge
git checkout -qf FETCH_HEAD

Submodule 'nupic.core' (git://github.com/numenta/nupic.core.git) registered for path 'nta'
Cloning into 'nta'...
fatal: reference is not a tree: 966a48f991318a4423656803dc083e66827b9385
Unable to checkout '966a48f991318a4423656803dc083e66827b9385' in submodule path 'nta'
CMake Error at CMakeLists.txt:346 (message):
  Updating submodules within /home/travis/build/numenta/nupic failed.

[utensil]

To solve this problem, the following steps might help:

1. numenta/nupic.core merges Independent Build Process nupic.core-legacy#47 --- it's fine becasue it's green by itself, but it seems review is not yet finished
2. Integration with nupic.core API #751 adds a commit update submodule reference to HEAD of numenta/nupic.core , NOT david-ragazzi/nupic-core --- thus we can put the newest numenta/nupic.core under numenta/nupic Travis tests
3. When Travis passes and all reviews are finished, nupic merges Integration with nupic.core API #751

This shouldn't break things.

[david-ragazzi]

To solve this problem, the following steps might help:

1. `numenta/nupic.core` merges numenta/nupic.core#47 --- it's fine becasue it's green by itself, but it seems review is not yet finished
2.  #751 adds a commit update submodule reference to HEAD of `numenta/nupic.core` , NOT `david-ragazzi/nupic-core` --- thus we can put the newest `numenta/nupic.core` under `numenta/nupic` Travis tests
3. When Travis passes and all reviews are finished, `nupic` merges #751 

This shouldn't break things.

@rhyolight @utensil's suggestion makes sense to me.. What do you think?

[rhyolight]

@david-ragazzi See numenta/nupic.core-legacy#47 (comment)

[david-ragazzi]

@david-ragazzi See numenta/nupic.core-legacy#47 (comment)

Thanks Matt!

[rhyolight]

numenta/nupic.core-legacy#47 is merged. You are unblocked to continue with your plan.

[scottpurdy]

@david-ragazzi @utensil @breznak - Thank you all for the time on this PR - I think this might be our most involved PR discussion to date!

I am not too familiar with CMake but it looks to me like the canonical method for including files is to have a hierarchical set of CMakeLists.txt files where the top level one simply includes the appropriate subdirs which each have their own CMakeLists.txt files. Would this be better than the macro that traverses all the subdirs?

I think it is important to get this merged asap so this isn't a blocking issue.

[david-ragazzi]

I am not too familiar with CMake but it looks to me like the canonical method for including files is to have a hierarchical set of CMakeLists.txt files where the top level one simply includes the appropriate subdirs which each have their own CMakeLists.txt files. Would this be better than the macro that traverses all the subdirs?

Yes, it's a canonical method, but not required or considered the best one.. To say the truth, I think that is a culture inherited from Autotools as this eases the conversion of existent MakeFiles spread over the directories to CMakeLists.txt.. Personally, I consider a painfull job transverse the subdirectories tree for trying understand the entire process.. My first version of CMake process used hierarchy, but when I simplified to a single file at top dir, the things ran fast (updating and maintenance were easier)!! Furthermore, we avoid create several CMakeLists even for empty dirs!

[chetan51]

First of all, thanks to @david-ragazzi, @utensil and @breznak for working on this, this is really great!

I'm testing it out now. When I run this command:

sudo pip install --allow-all-external --allow-unverified PIL --allow-unverified psutil -r external/common/requirements.txt

I see:

cc -fno-strict-aliasing -fno-common -dynamic -arch x86_64 -arch i386 -g -Os -pipe -fno-common -fno-strict-aliasing -fwrapv -mno-fused-madd -DENABLE_DTRACE -DMACOSX -DNDEBUG -Wall -Wstrict-prototypes -Wshorten-64-to-32 -DNDEBUG -g -fwrapv -Os -Wall -Wstrict-prototypes -DENABLE_DTRACE -arch x86_64 -arch i386 -pipe -DHAVE_LIBJPEG -DHAVE_LIBZ -DHAVE_LIBTIFF -I/System/Library/Frameworks/Tcl.framework/Versions/8.5/Headers -I/System/Library/Frameworks/Tk.framework/Versions/8.5/Headers -I/opt/X11/include/freetype2 -I/private/tmp/pip_build_root/pillow/libImaging -I/System/Library/Frameworks/Python.framework/Versions/2.7/include -I/opt/X11/include -I/usr/local/include -I/usr/include -I/System/Library/Frameworks/Python.framework/Versions/2.7/include/python2.7 -c _imaging.c -o build/temp.macosx-10.9-intel-2.7/_imaging.o

clang: error: unknown argument: '-mno-fused-madd' [-Wunused-command-line-argument-hard-error-in-future]

clang: note: this will be a hard error (cannot be downgraded to a warning) in the future

error: command 'cc' failed with exit status 1

----------------------------------------
Cleaning up...
Command /usr/bin/python -c "import setuptools, tokenize;__file__='/private/tmp/pip_build_root/pillow/setup.py';exec(compile(getattr(tokenize, 'open', open)(__file__).read().replace('\r\n', '\n'), __file__, 'exec'))" install --record /tmp/pip-JFXYUa-record/install-record.txt --single-version-externally-managed --compile failed with error code 1 in /private/tmp/pip_build_root/pillow
Storing debug log for failure in /Users/csurpur/Library/Logs/pip.log

[rhyolight]

@chetan51 Scott saw that too, but he had the same problem on master, so I don't think it's related to this PR. He was going to create a new issue for that problem.

[david-ragazzi]

🍏

Who will merge?

[chetan51]

After building, I tried running the Python tests:

build/scripts - [46835bf...] » make tests_run_all
Scanning dependencies of target tests_run_all
[100%] Python tests + swarming (requires DB)
Traceback (most recent call last):
  File "/Users/csurpur/Development/nupic/build/release/bin/run_tests.py", line 254, in <module>
    os.chdir(os.getenv('NUPIC'))
TypeError: coercing to Unicode: need string or buffer, NoneType found
make[3]: *** [CMakeFiles/tests_run_all] Error 1
make[2]: *** [CMakeFiles/tests_run_all.dir/all] Error 2
make[1]: *** [CMakeFiles/tests_run_all.dir/rule] Error 2
make: *** [tests_run_all] Error 2
build/scripts - [46835bf...] » make tests_run
Scanning dependencies of target tests_run
[100%] Python tests
Traceback (most recent call last):
  File "/Users/csurpur/Development/nupic/build/release/bin/run_tests.py", line 254, in <module>
    os.chdir(os.getenv('NUPIC'))
TypeError: coercing to Unicode: need string or buffer, NoneType found
make[3]: *** [CMakeFiles/tests_run] Error 1
make[2]: *** [CMakeFiles/tests_run.dir/all] Error 2
make[1]: *** [CMakeFiles/tests_run.dir/rule] Error 2
make: *** [tests_run] Error 2

[david-ragazzi]

@chetan51

File "/Users/csurpur/Development/nupic/build/release/bin/run_tests.py", line 254, in
os.chdir(os.getenv('NUPIC'))

You forgot set $NUPIC..

[scottpurdy]

My build fails. I don't know if this is an issue with this PR though since master doesn't build for me either (albeit with a different error that this PR fixes).

See my build attempt here:
http://pastebin.com/XY6EpVWA

This was after I resolved the install of externals that Chetan saw a problem with. Resolved with this method:
http://stackoverflow.com/a/22322645

[rhyolight]

@chetan51 Like David said, the README has updated instructions on what to add to your .bashrc. We're hoping to remove this stuff soon, but that is another issue.

[chetan51]

@david-ragazzi Oops, I copied this line verbatim from the README:

export NUPIC=$REPOSITORY

Of course, $REPOSITORY is set to nothing.

Once I set that correctly, some of the Python tests passed. But now I'm seeing this error:

http://pastie.org/9004269

[rhyolight]

😧 So close! But this is why we do code reviews! Thanks @scottpurdy. I'll have to defer to ( @david-ragazzi | @utensil | @breznak ) for debugging that error.

[rhyolight]

@chetan51 said

Once I set that correctly, some of the Python tests passed. But now I'm seeing this error:

http://pastie.org/9004269

Thanks, I was not seeing that error in integration tests before I merged in master. I'm running them again locally now.

[scottpurdy]

I don't think these should be blocking issues. The data path can be solved by setting the environment variable until we figure out why it doesn't work without it. And my issue needs to be resolved but it will be easier to do as a follow up PR without so much baggage.

[rhyolight]

All tests passed for me.

[david-ragazzi]

I don't think these should be blocking issues. The data path can be solved by setting the environment variable until we figure out why it doesn't work without it. And my issue needs to be resolved but it will be easier to do as a follow up PR without so much baggage.

Well, so there's not problem in merge.. hehe

[breznak]

We're missing -msse2 flag

[rhyolight]

Ok guys, any further issues should be logged as separate tickets and resolved in different PRs. I believe we have enough of a consensus to move ahead with this merge.

[scottpurdy]

@david-ragazzi - A couple more questions:

1. It looks like the core build is happening when I run the cmake command. I thought that command was just supposed to generate the make files and then the make command did the actual compilations?
2. Can we change the README to remove $REPOSITORY and just use $NUPIC? Since we have to set $NUPIC anyway, $REPOSITORY is redundant and confusing (and they should never set that since it is too generic).

[breznak]

@scottpurdy

1. It looks like the core build is happening when I run the cmakecommand. I thought that command was just supposed to generate the make
   files and then the make command did the actual compilations?

We've discussed that somewhere in comments for this PR - I thought the
same. Reasoning was the simplification and possible future move to just
binding a libnupiccore.so

confirm @brezenak issue 2.1

I thought nupic.core submodule update can be done in CMake stage, as you
did, though it seems weird to build nupic.core in CMake stage.

But I finally understood: if building nupic.core is done in make stage, we
would have to generate Makefiles explicitly depends on the source code of
nupic.core, which could be in the way to remove this building and depend on
nupic.core binary release later.

So I guess it's better to leave building in CMake stage for easier removal.

1. Can we change the README to remove $REPOSITORY and just use $NUPIC?
   Since we have to set $NUPIC anyway, $REPOSITORY is redundant and confusing
   (and they should never set that since it is too generic).

+1 we use NUPIC anyway, so stick with it.

PS: how do I link to existing comments on GH?

[chetan51]

I opened an issue: #784

[david-ragazzi]

Can we change the README to remove $REPOSITORY and just use $NUPIC? Since we have to set $NUPIC anyway, $REPOSITORY is redundant and confusing (and they should never set that since it is too generic).

Agreed!

[david-ragazzi]

It looks like the core build is happening when I run the cmake command. I thought that command was just supposed to generate the make files and then the make command did the actual compilations?

The question is that we need a compiled static library before generate files at Nupic.. However, I think this deserve further discussion..

[scottpurdy]

@breznak - regarding linking to a specific comment, the time that the comment was posted (like "20 minutes ago", right after the commenter's name) is a link so you can get it from there.