ola
ola copied to clipboard
OpenLightingProject
patches to OLA python, so library works in both python2 and python3
Just wanted to submit my patches to OLA, so the Python API works also for python3.
Have been using this for a while in Raspbian GNU/Linux 9.11 (stretch), and has worked for me so far for both DMX and RDM. However, it has not been extensively tested with all possible edge cases.
Typically, what I do is just:
pi@raspberrypi:~ $ sudo ln -s /usr/lib/python2.7/dist-packages/ola /usr/lib/python3/dist-packages/
... and then just patch the files in /usr/lib/python2.7/dist-packages/ola; here I've done the same changes in the OLA source tree.
Thanks. Please take a look at #1605 which hopefully will be merged soon, but has been built up over time with a lot of fixes.
Your PR has some additional changes that aren't in 1605 (RDMMessage in OlaClient and some PidStore changes) that ought to be captured, though I'm afraid that still isn't all changes in the category of string/byte array fixes. That's one of the most compelling arguments for making a complete break from 2.7 and writing only for 3.
There's been a lot of discussion about the whole py2 vs py3 thing. I think the current plan is to do one more point release without breaking anything (including 2.6, which gives even more constraints) and then move on in the next major release, hopefully soon. 1605 is also on top of 0.10, which is where the releases come from at the moment.
I'd be happy to take those changes not in 1605 and add them to it, if you'd like.
Thanks @brucelowekamp ,
Please take a look at #1605 which hopefully will be merged soon, but has been built up over time with a lot of fixes.
Oh, buy - there was a lot more to the whole thing than I expected; I guess I've had quite a limited use of OLA to get away with just the changes in this PR.
That's one of the most compelling arguments for making a complete break from 2.7 and writing only for 3.
I must admit, I got rather irritated back in the day with the whole python2/python3 thing - I guess I got used to it now, but I still rather fancy code that tends to work in both :)
Of course, the project has to follow whatever is the best for most of its users, so if a library split comes eventually, I'll adapt to that.
I'd be happy to take those changes not in 1605 and add them to it, if you'd like.
Sure, that would be great!
Thanks @sdbbs !
Your PR has some additional changes that aren't in 1605 (RDMMessage in OlaClient and some PidStore changes) that ought to be captured, though I'm afraid that still isn't all changes in the category of string/byte array fixes. That's one of the most compelling arguments for making a complete break from 2.7 and writing only for 3.
I'm personally probably against this I think @brucelowekamp . If nothing else I worry it will generate just as many issues with people running the wrong version of Python. This is assuming it's not too arduous to do both, such as the version checks in here.
I'd be happy to take those changes not in 1605 and add them to it, if you'd like.
Yes please @brucelowekamp .
Thanks @sdbbs !
Your PR has some additional changes that aren't in 1605 (RDMMessage in OlaClient and some PidStore changes) that ought to be captured, though I'm afraid that still isn't all changes in the category of string/byte array fixes. That's one of the most compelling arguments for making a complete break from 2.7 and writing only for 3.
I'm personally probably against this I think @brucelowekamp . If nothing else I worry it will generate just as many issues with people running the wrong version of Python. This is assuming it's not too arduous to do both, such as the version checks in here.
I'd be happy to take those changes not in 1605 and add them to it, if you'd like.
Yes please @brucelowekamp .
@peternewman I can pull in the changes, but I looked at the code a bit, and I think there are others. I also am not completely sure whether pb generates different code for python 3 vs 2, so can’t be sure the string behavior won’t change again. I don’t think it hurts to incorporate these changes if desired to get 1605 in, but I think a full RDM check needs to be done in a full python 3 build, and there are quite a few changes in that area that are in master.
Hi @sdbbs / @brucelowekamp ,
How do we want to proceed with this now #1605 is finally in? Shall we get this PR retargetted at 0.10 or start over cherry-picking some commits?
@peternewman I can pull in the changes, but I looked at the code a bit, and I think there are others. I also am not completely sure whether pb generates different code for python 3 vs 2, so can’t be sure the string behavior won’t change again.
Are you happy to do those bits still @brucelowekamp ? Or are you hoping someone else will help?
I don’t think it hurts to incorporate these changes if desired to get 1605 in, but I think a full RDM check needs to be done in a full python 3 build, and there are quite a few changes in that area that are in master.
Do you think there are functional changes in master with RDM? I know we've added lots of tests, but I think they're relatively simple Python code wise. I think most of it applies nicely still, so we should theoretically be able to fix 0.10, merge that into master and then fix the new bits in master.
@peternewman I took a look at this patchset awhile ago, and it really gets to the heart of the issue that how strings are represented is fundamentally different in 2 vs 3. The biggest unresolved problem I had in the RDM python 3 patches I had was also in an area where string encoding came up.
What I'd really like to do is take a look at how the code behaves when built with python 3 vs python 2 once the protobuf 3.11 change is in. I looked at various protobuf CHANGES files and couldn't find anything indicating there was a difference, but I would like to make sure we have a full py3 build from a more current protobuf so we aren't trying to patch something odd about taking a py2 build/protobuf generated files and then patching them to work with py3.
In an ideal world, we could also minimize the number of times (if any) different code is run for py2 vs py3, though I'm not completely sure how much refactoring of PidStore would be required to make that happen. Honestly if it's not possible to minimize those I would push for dropping py2 entirely.
I can probably give it a look maybe this weekend assuming the 3.11 protobuf code merges as well. My last check on that issue was you were mostly looking at some odd buildsystem issues, so it seems like playing around with it probably wouldn't be wasted.
Bruce
@peternewman I took a look at this patchset awhile ago, and it really gets to the heart of the issue that how strings are represented is fundamentally different in 2 vs 3. The biggest unresolved problem I had in the RDM python 3 patches I had was also in an area where string encoding came up.
Thanks for the summary @brucelowekamp .
What I'd really like to do is take a look at how the code behaves when built with python 3 vs python 2 once the protobuf 3.11 change is in. I looked at various protobuf CHANGES files and couldn't find anything indicating there was a difference, but I would like to make sure we have a full py3 build from a more current protobuf so we aren't trying to patch something odd about taking a py2 build/protobuf generated files and then patching them to work with py3.
Makes lots of sense.
In an ideal world, we could also minimize the number of times (if any) different code is run for py2 vs py3, though I'm not completely sure how much refactoring of PidStore would be required to make that happen. Honestly if it's not possible to minimize those I would push for dropping py2 entirely.
I assume you mean the individual conversions of string/byte array or whatever to/from each other?
TBH @sdbbs changes look like they'd hit most of the interface points where we're converting to/from the wire format. I'd hope the other bits should be less crucial if it's fixed at the core (although I guess there may be some Raw type methods in the RDM tests too).
I can probably give it a look maybe this weekend assuming the 3.11 protobuf code merges as well. My last check on that issue was you were mostly looking at some odd buildsystem issues, so it seems like playing around with it probably wouldn't be wasted.
Thanks. Yeah looking at the current state of #1630 it's working fine for some on Travis and for me on an older build, so it's just minor technicalities (a bit like a lot of the Python 3 PR was). It's also essentially just a copy of #1620 which is already merged into master.
@peternewman Sorry for going away for so long. Trying to look at this again. My concern about the patch is that e.g. in OlaClient.py there is a patch for _RDMMessage and I only see that being used in get and set. But I see similar code with data in RDMFrame as used in RDMResponse, and I don't see that it's using RDMMessage in that path (though I've only looked at it for a bit and am entirely open to having missed that everything at some point goes through RDMMessage)
Anyway, what I'd really like is a test suite that exercises most of these flows so they can be tested in py2 and py3. I know in the main py3 issue discussion you said you normally just let the rdm test suite catch problems, but it isn't complete. Is it complete enough that it would exercise all of these flows in the python api? Or would using the rdm tools in separate processes to talk back and forth be necessary to exercise everything?
@peternewman Sorry for going away for so long.
Don't worry about it.
Trying to look at this again. My concern about the patch is that e.g. in OlaClient.py there is a patch for _RDMMessage and I only see that being used in get and set. But I see similar code with data in RDMFrame as used in RDMResponse, and I don't see that it's using RDMMessage in that path (though I've only looked at it for a bit and am entirely open to having missed that everything at some point goes through RDMMessage)
I think that's because _RDMMessage (and SendRawRDMDiscovery) are being used to send requests, whereas RDMFrame/RDMResponse are being used to handle responses to those requests.
Anyway, what I'd really like is a test suite that exercises most of these flows so they can be tested in py2 and py3. I know in the main py3 issue discussion you said you normally just let the rdm test suite catch problems, but it isn't complete. Is it complete enough that it would exercise all of these flows in the python api? Or would using the rdm tools in separate processes to talk back and forth be necessary to exercise everything?
It's probably complete enough running the RDM tests, but it's probably not perfect I think it's fair to say.
Could we do something in the same vein as this, making a Mock _stub and feeding it the binary data we expect beforehand and then validating that it passes in the expected data? https://github.com/OpenLightingProject/ola/blob/master/common/rdm/RDMAPITest.cpp