-
Notifications
You must be signed in to change notification settings - Fork 207
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
ImportError (google.protobuf) with rdm_test_server.py #1599
Comments
Hi @ssilverman , We are having the same issue too, you can see our progress in #1584 . We're currently doing: I think Homebrew have switched to Python 3 only for Protobuf, see #1538 for our progress on that front. Although did ./configure succeed for you? It may just be a path issue in that case. Could you post your config.log (probably before trying to re-run it). |
Before I try anything I'll gladly send you the |
I don't know specifically, but this looks relevant: Other than that, it will likely be in an ola or olad folder if you can find any config.log files on your machine. |
There's no |
@ssilverman : Could you please retry, now that #1605 has been merged? It might have fixed your issue :) |
It was never complete, but if you're interested I had some of the work done
in the py3-build branch of https://github.com/brucelowekamp/ola.git.
There are some imports and other things that I think were necessary beyond
the conversion of print and xrange (which is of course the bulk of the
changes). I *think* this was the result of running futurize and a couple
other tools over the codebase. But I remember there being some unicode
stuff in a web module that I couldn't sort out, and there were definitely a
few fun things like:
./tools/rdm/TestDefinitions.py: ALL_MODES = ALLOWED_MODES + [3] +
range(0x80, 0xe0)
that didn't get converted automatically to list(range), and I realized I
needed to go back and find them. That was when I narrowed the scope to
just the API...
Bruce
…On Wed, Nov 18, 2020 at 7:32 PM Peter Newman ***@***.***> wrote:
The RDM tests still don't run under Python 3 due to a few outstanding
issues @kripton <https://github.com/kripton> (xrange being one of the
easier ones), so while it may have progressed things and #1655
<#1655> will have done
too, the tests themselves still aren't quite there unfortunately.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1599 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADNHN5KRMPXNZF7RGFFGRG3SQRRSDANCNFSM4JRA4RUQ>
.
|
Thanks @brucelowekamp . I've also done the print changes in #1685 which I'll merge in as it didn't seem to hurt and seemed to be sufficient to get the compileall stuff to work, although flake8 needs more work, and actually running seems to need more still in some cases. I went for a slightly different xrange/range solution as in theory it should be more efficient on python2 I think: |
IMHO I would rather change the xranges to range and
from builtins import range
(or just from future import * so no one needs to think about it)
but if you want to leave the xranges in place, then
from past.builtins import xrange
https://python-future.org/compatible_idioms.html#xrange
I think is preferable.
IMHO it’s better to use range everywhere b/c you can wind up thinking about
xrange and range as different things, which they are in python 2 but not
python 3 (unless you from past.builtins import range, but let’s not go
there), and it’s better to think about them as the same function and write
python 3 code moving forward.
Bruce
On Sat, Nov 21, 2020 at 5:29 PM Peter Newman ***@***.***> wrote:
Thanks @brucelowekamp <https://github.com/brucelowekamp> . I've also done
the print changes in #1685
<#1685> which I'll merge
in as it didn't seem to hurt and seemed to be sufficient to get the
compileall stuff to work, although flake8 needs more work, and actually
running seems to need more still in some cases.
I went for a slightly different xrange/range solution as in theory it
should be more efficient on python2 I think:
https://github.com/OpenLightingProject/ola/pull/1685/files#diff-5b3f8e5df8e5a430e32762b12ed99f846058b721ae6049489f7a6058fbd8f45eR25-R31
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1599 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADNHN5NNSX3GB7DJTFWGOYDSRA5NDANCNFSM4JRA4RUQ>
.
--
Sent from my mobile
|
I thought theoretically xrange should be more efficient on Python 2 from my understanding? Although how much difference it makes in say the licence check when it's just four values I don't really know. From what I can tell builtins isn't core either, compared to future, i.e. it's another dependency?
From my, admittedly very quick look, past.builtins appeared to be a separate module which we're then requiring people to have for compatibility.
Yeah I can see the logic behind that, I guess I was initially driven by doing it as a quick win on a few bits of code. I certainly haven't made the whole codebase pass yet. If the performance issue is fairly minor then doing a find/replace of xrange to range seems pretty trivial too. |
I guess the question with performance is whether it matters there. For the
tests, code generators, etc, I would assume not. If some of the rdm code
is used where it matters, might be worth looking at. My impression was
that most of the code was for testing and didn't appear to be performance
sensitive, but you're a better judge of that. I'm amazed at how fast
python is even with stupid array manipulations (I've never switched my code
to numpy) so I would be surprised if it matters.
As future can be installed with pip, IMHO it's not a concern. It's not
like needing to find an ancient protobuf binary ;) I might have some
slight concern with the fact that while it is still being maintained, it's
obvious no one is aggressively trying to maintain full forward
compatibility in 3.x, but I would assume that's definitely not a concern
with using future (since it really only does something in py2, which isn't
changing) and I doubt it would be a problem with the small set of features
ola might import from past.builtins.
Bruce
…On Sat, Nov 21, 2020 at 11:10 PM Peter Newman ***@***.***> wrote:
IMHO I would rather change the xranges to range and
from builtins import range
(or just from future import * so no one needs to think about it)
I thought theoretically xrange should be more efficient on Python 2 from
my understanding? Although how much difference it makes in say the licence
check when it's just four values I don't really know.
From what I can tell builtins isn't core either, compared to *future*,
i.e. it's another dependency?
but if you want to leave the xranges in place, then
from past.builtins import xrange
https://python-future.org/compatible_idioms.html#xrange
I think is preferable.
From my, admittedly very quick look, past.builtins appeared to be a
separate module which we're then requiring people to have for compatibility.
IMHO it’s better to use range everywhere b/c you can wind up thinking about
xrange and range as different things, which they are in python 2 but not
python 3 (unless you from past.builtins import range, but let’s not go
there), and it’s better to think about them as the same function and write
python 3 code moving forward.
Yeah I can see the logic behind that, I guess I was initially driven by
doing it as a quick win on a few bits of code. I certainly haven't made the
whole codebase pass yet. If the performance issue is fairly minor then
doing a find/replace of xrange to range seems pretty trivial too.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1599 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADNHN5PHLE4SVBURUSBLDRLSRCFJVANCNFSM4JRA4RUQ>
.
|
Update (including all the messages in case others search for anything similar): After installing
I installed
From
It looks like the search directory needs to append that "_5"? (A simple |
Yeah we're only using numpy for some stats, not for performance reasons. You're generally right, I'm sure people would like to run tests quickly, especially if they're iterating, but it's only the stuff in the python folder that might be used in a realtime way (e.g. the DMX and RDM manipulation during a show, and generally only the DMX side of that).
It's not so much pip, it's more that means they all need to be in the various packaging systems people package OLA for. I think I was using 3.1.0 as it was consistent across the Ubuntu and Mac packages (probably mostly limited by the old Ubuntu version, but we've moved on since then so on the Travis Bionic we're using the built in deb packaged version with Python 2 (and only not on Python 3 as I think the way I'm switching Python misses the links to the relevant dist packages for some reason, probably mixing pip/pyenv and deb). |
Good spot @ssilverman . This feels like a bug with Homebrew given that file just contains the contents of piddatadir which itself is just datadir which is just datarootdir which is just prefix/share: Possibly it didn't get updated when the bottle got built. This will go away when this goes in, but just because the variant gets reset: Do you want to report this to them given you've got a Mac to test it on. Feel free to @mention me/this in it too. |
Does the olad web interface work in this setup too? I guess you only tested after adding the symlink. |
@ssilverman FYI the RDM tests should finally work on Python 3 in #1761 if you want to take a look? |
Closing this as it should have been resolved by #1761 . Let us know if you find any outstanding RDM Python bugs. |
When I try to run
rdm_test_server.py
, I get this:I'm using homebrew to install ola. I've tried reinstalling protobuf and ola, but I get the same error.
System: OSX 10.14.6
The text was updated successfully, but these errors were encountered: