-
Notifications
You must be signed in to change notification settings - Fork 1k
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
WIP Python fixes #383
base: master
Are you sure you want to change the base?
WIP Python fixes #383
Conversation
Any feedback on those changes? |
Sorry for the radio silence, don't have time to check it yet. I'll review and merge in the next few days. |
push |
Thanks Nico. I'll make sure I give a pass to this tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NicoHood for the PR. I added a few comments. And also some high level comments:
-
I think it's better if you and also finish your three TODOs in this PR.
-
I'm a little bit hesitating moving things like snowboydecoder.py and resources to swig/Python3 or swig/Python. That's a bit change. If we are going to do that, could you document that in the README? Also, swig/Python3/resources is just a softlink to the actual file. When you make the Python package, could you make sure you copy the directory following the soft link (so that you could copy the actual files)?
examples/Python3/demo.py
Outdated
detector = snowboydecoder.HotwordDetector(model, sensitivity=0.5) | ||
print('Listening... Press Ctrl+C to exit') | ||
detector = snowboydecoder.HotwordDetector(model, sensitivity=0.5) | ||
detector.detector.ApplyFrontend(False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a new param for apply_frontend
. We can now set it through initialization.
@@ -35,7 +35,7 @@ else | |||
CXX := g++ | |||
PYINC := $(shell python-config --cflags) | |||
PYLIBS := $(shell python-config --ldflags) | |||
SWIGFLAGS := -shared | |||
SWIGFLAGS := -Wl,-O1,--as-needed -shared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the -O1 option here asks the compiler to do level 1 optimizations, might be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont understand. What do you mean? Was this just a "hey, thats good!" or are you requesting some change? I am already using -O1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh OK sorry, mis-read it... I thought "SWIGFLAGS := -Wl,-O1,--as-needed -shared" was the original line.
@@ -39,14 +39,14 @@ else | |||
CXX := g++ | |||
PYINC := $(shell python3-config --cflags) | |||
PYLIBS := $(shell python3-config --ldflags) | |||
SWIGFLAGS := -shared | |||
SWIGFLAGS := -Wl,-O1,--as-needed -shared |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, the -O1 option here asks the compiler to do level 1 optimizations, might be helpful
I have updated my PR, however I cannot get the python2 examples working in general. This seems to not be a problem of this PR, it just doesnt work on my system. Another thing I noticed is that the decoder file differs for python2 and 3, which we want to avoid:
|
@chenguoguo I've merged the other PR into this one. Could you please give it a test? For me the python2 examples dont work at all, but they also did not work before the PR for me. Beside this the snowboydecoder.py for python2 and 3 are out of sync. You should know best which one of the two is the most up to date. Those should be merged into a single file, that is simpler to maintain. The same for the examples itself. If the import for python2 gets fixed, there is no need to have examples for python2 or 3 separated. |
This PR tries to fix several problem with the Python package:
TODO:
Adapt changes with Python2We should make all examples rather Python2 and 3 compatible or remove python2 support/examples completely.Explain pythonpath setting in the readmeDoneAlso fix remaining demos ( NumHotwords() incorrect #381 blocking)I would like to have some feedback, then I am willing to fix the remaining TODOs