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

Validate python nupic is still compatible with our nupic.core c++ bindings #137

Closed
3 tasks done
breznak opened this issue Dec 1, 2018 · 6 comments
Closed
3 tasks done
Labels
help wanted Extra attention is needed high tests

Comments

@breznak
Copy link
Member

breznak commented Dec 1, 2018

c++ provides nupic.bindings; nupic(py) is the main (currently only) user of it, verify we didn't break any API.

Even local test is ok for now.
TODO

Relevant: #104 #81

@breznak breznak added this to the Repo setup milestone Dec 1, 2018
This was referenced Dec 1, 2018
@breznak
Copy link
Member Author

breznak commented Dec 1, 2018

I've installed nupic from pip only (pip install nupic), pulls nupic.bindings with it too.
Got some errors? I don't understand if are errors:

================================================================== pytest-warning summary ===================================================================
WC1 /mnt/store/devel/HTM/nupic-source/tests/unit/nupic/engine/network_test.py cannot collect test class 'TestNode' because it has a __init__ constructor
WC1 /mnt/store/devel/HTM/nupic-source/tests/unit/nupic/frameworks/opf/htmpredictionmodel_classifier_helper_test.py cannot collect test class 'TestOptionParser' because it has a __init__ constructor
WC1 /mnt/store/devel/HTM/nupic-source/tests/unit/nupic/regions/knn_anomaly_classifier_region_test.py cannot collect test class 'TestOptionParser' because it has a __init__ constructor
WC1 /mnt/store/devel/HTM/nupic-source/tests/unit/nupic/support/decorators_test.py cannot collect test class 'TestParentException' because it has a __init__ constructor
WC1 /mnt/store/devel/HTM/nupic-source/tests/unit/nupic/support/decorators_test.py cannot collect test class 'TestChildException' because it has a __init__ constructor
========================================== 713 passed, 17 skipped, 8 xfailed, 5 pytest-warnings in 188.62 seconds ===

@breznak
Copy link
Member Author

breznak commented Dec 1, 2018

Replacing binary bindings with ones from our repo:

pip uninstall nupic.bindings
python $NUPIC_CORE/setup.py install

Many PY tests fail due to capnp remains:

capnp
__________________________________________ ERROR collecting tests/unit/nupic/regions/sdr_classifier_region_test.py __________________________________________
tests/unit/nupic/regions/sdr_classifier_region_test.py:27: in <module>
    from nupic.encoders import MultiEncoder
../pyenv/lib/python2.7/site-packages/nupic/encoders/__init__.py:23: in <module>
    from random_distributed_scalar import RandomDistributedScalarEncoder
../pyenv/lib/python2.7/site-packages/nupic/encoders/random_distributed_scalar.py:38: in <module>
    from nupic.encoders.random_distributed_scalar_capnp import (
capnp/lib/capnp.pyx:3947: in capnp.lib.capnp._Loader.load_module (capnp/lib/capnp.cpp:75560)
    ???
capnp/lib/capnp.pyx:3926: in capnp.lib.capnp.load (capnp/lib/capnp.cpp:75048)
    ???
capnp/lib/capnp.pyx:3247: in capnp.lib.capnp.SchemaParser.load (capnp/lib/capnp.cpp:65930)
    ???
E   KjException: src/capnp/compiler/node-translator.c++:2090: context: member.name = random
E   /mnt/store/devel/HTM/pyenv/lib/python2.7/site-packages/nupic/encoders/random_distributed_scalar.capnp:2: failed: Import failed: /nupic/proto/RandomProto.capnp
________________________________________________ ERROR collecting tests/unit/nupic/regions/tm_region_test.py ________________________________________________
ImportError while importing test module '/mnt/store/devel/HTM/nupic-source/tests/unit/nupic/regions/tm_region_test.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/unit/nupic/regions/tm_region_test.py:33: in <module>
    from nupic.regions.tm_region import TMRegion
../pyenv/lib/python2.7/site-packages/nupic/regions/tm_region.py:32: in <module>
    from nupic.algorithms import (anomaly, backtracking_tm, backtracking_tm_cpp,
../pyenv/lib/python2.7/site-packages/nupic/algorithms/__init__.py:28: in <module>
    from nupic.bindings.algorithms import svm_01, svm_dense
E   ImportError: cannot import name svm_01
================================================================== pytest-warning summary ===================================================================
WC1 /mnt/store/devel/HTM/nupic-source/tests/unit/nupic/support/decorators_test.py cannot collect test class 'TestParentException' because it has a __init__ constructor
WC1 /mnt/store/devel/HTM/nupic-source/tests/unit/nupic/support/decorators_test.py cannot collect test class 'TestChildException' because it has a __init__ constructor
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 47 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
======================================================== 2 pytest-warnings, 47 error in 1.65 seconds

This needs to be fixed/aligned in the community py repo.

@dkeeney
Copy link

dkeeney commented Dec 1, 2018 via email

@breznak
Copy link
Member Author

breznak commented Dec 1, 2018

First, synch the community nupic .py code with the numenta repository. At least get as close to their code as possible.

We made no changes, so that will be simple.

Then we could go through the community nupic .py code and fix the capnp and at the same time make the modifications so that it could run with both Python2.7 and Python3.

I'd do one at a time, just capnp now. It'll be lot of changes but hopefully not too difficult:

  • we could either write replacement for pycapnp back to pickle (should be in git history)
  • or just always use cpp version for serialization (that is not a pure python solution, but a quick hack)

When that is finished, our .py code is different from the numenta repository and the possibility of our .py code being absorbed back into numenta becomes less likely.

I see, you're aiming to merge Py3 code to numenta's? Well, 2 ideas here:

  • make the pycapnp chages and then we squash&merge the PR, being one commit, that will be easy to revert for numenta inclusion.
  • since both Numenta's nupic .py, and .cpp repos are in maintanance mode only, and they don't want to do any changes to them; and as you mentioned in SpatialPooler - SDR integration #121 - people are not using our repo.
    My aim with this community fork is to keep a live, developing version of the numenta's algorithms. Py3 support would be a good competitive advantage (for users to come to our repo);

TL;DR: same as with capnp removal, I'd keep the burden on numenta, if they want to merge back the Py3 changes.

@breznak
Copy link
Member Author

breznak commented Dec 20, 2018

For anyone interested in helping, please start looking at
htm-community/nupic.py#4

  • I can run the tests on
  • stop getting SIGSEGV
  • start repairing broken tests & classes
    • best case: fix
    • rewrite to use py-only alternative
    • remove (and document)
    • there are lots of failing cases to choose from, so we can start tackling that one-by-one, make a pick 😉

@ctrl-z-9000-times
Copy link
Collaborator

At this point the answer to the question "is our fork compatible with nupic.py?" is clearly no, we are not compatible. There have been several large API breaking changes, including:

  • Removed capnproto, sparse matixes,
  • Algorithms now use SDRs instead of raw data arrays,
  • Several encoders changed API to use a parameter structure, also some of their argument names changed.

At the time, each of these changes was justified. Now the goal is to deliver a cohesive software package. To this end we should incorporate the contents of nupic.py into this repository by merging it and fixing the compatibility issues. See issue #216.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed high tests
Projects
None yet
Development

No branches or pull requests

3 participants