Closed
Bug 1294230
Opened 8 years ago
Closed 7 years ago
mozillapulse bindings are incompatible with python 3
Categories
(Webtools :: Pulse, defect)
Webtools
Pulse
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: amiyaguchi, Assigned: rhubscher)
Details
Attachments
(1 file, 3 obsolete files)
23.63 KB,
patch
|
mcote
:
review+
leplatrem
:
feedback+
|
Details | Diff | Splinter Review |
A clean install of mozillapulse on python 3.5.1 against the example application will immediately fail with the following exception.
> Traceback (most recent call last):
> File "aggregate.py", line 4, in <module>
> from mozillapulse.config import PulseConfiguration
> File "/Users/amiyaguchi/Work/sandbox/venv/lib/python3.5/site-packages/mozillapulse/__init__.py", line 9, in <module>
> reload(sys)
> NameError: name 'reload' is not defined
Mozilla pulse should support python 3 bindings. I want to write a script for my own self automation driven by pulse events rather than polling, but not having python 3 support is a turn-off for using this library.
I also want to mention that the python bindings aren't as reliable as I'd like it to be out of the box. In a very simple test of the mozillapulse library, I only managed to collect 50000 messages in a 2 hour period against the exchange/taskcluster-*/ routes before the backfill caused it to close.
Comment 1•8 years ago
|
||
Yes, Python 3 support is a great idea. Sadly contributions to mozillapulse are fairly rare, but if you'd like to add it I'd happily review your patch. The reliability is another question, but there's nothing inherently unstable in mozillapulse to my knowledge. It's basically a thin wrapper and set of convenience functions around kombu. If you'd like to post about your problems to mozilla.tools.pulse, I would be happy to help you out there, since it's probably just a question of usage.
Comment 2•7 years ago
|
||
Comment 3•7 years ago
|
||
Hi Mark! I'm interested in porting MozillaPulse to Python3. I submitted a patch, but what is your preferred workflow? Thanks!
Flags: needinfo?(mcote)
Updated•7 years ago
|
Assignee: nobody → mathieu
Comment 4•7 years ago
|
||
Comment on attachment 8863746 [details] [diff] [review] Port to Python 3 I will take a look. Thanks!
Flags: needinfo?(mcote)
Attachment #8863746 -
Flags: review?(mcote)
Comment 5•7 years ago
|
||
Comment on attachment 8863746 [details] [diff] [review] Port to Python 3 Hi, thanks for the patch! I confirmed that the consumer functionality is working; however, publishing doesn't seem to work. There were some local import errors (should be easy to fix). The tests also don't run under Python 3, since they are trying to import some Python 2-specific packages (like Queue), in addition to the local-import errors with publishing. Probably not much work to fix up these issues.
Attachment #8863746 -
Flags: review?(mcote) → review-
Comment 6•7 years ago
|
||
Oh please set me as reviewer when you post a new patch. :) Thanks!
Assignee | ||
Comment 7•7 years ago
|
||
This patch is: - Making sure the files are pep8 compliants - Add support for Python3 with the absolute_imports fixes. - Add a tox configuration to be able to run tests on py27, py35 and py36 locally. You can also view this patch on github here: https://github.com/Natim/MozillaPulse/pull/2
Attachment #8863746 -
Attachment is obsolete: true
Attachment #8869979 -
Flags: review?(mcote)
Attachment #8869979 -
Flags: feedback?(mathieu)
Comment 8•7 years ago
|
||
Hi, thanks, those are some good improvements! However, running test/runtests.py still fails, again due to it trying to import the Queue module. Is there something I'm missing?
Comment 9•7 years ago
|
||
Ah I see the tox work you did is to get the unit tests working. The runtests.py file I referred to contains separate, Vagrant-based, integration-type tests, although admittedly they don't work automatically right now. However, we should ensure they continue to work in all support Python versions. We should also get them working automatically (see, for example, the Docker-based PulseGuardian tests at https://github.com/mozilla/pulseguardian), but as a separate bug (if you're interested :).
Comment 10•7 years ago
|
||
Oh, natim, did you take this over from leplatrem? It's customary to give the assignee some time to post a new revision, as they might already be working on it. Unless you two coordinated elsewhere.
Comment 11•7 years ago
|
||
Comment on attachment 8869979 [details] [diff] [review] port_to_python3.patch I'm r-ing this for the issues in comment 9. It's really close though.
Attachment #8869979 -
Flags: review?(mcote) → review-
Assignee | ||
Comment 12•7 years ago
|
||
Sorry I missed your message because I wasn't following the bug. I fixed it. > running test/runtests.py still fails, again due to it trying to import the Queue module Is there a way for me to run the failing tests? Is it documented somewhere? > did you take this over from leplatrem Yes we have coordinated. > We should also get them working automatically but as a separate bug Ok sure. I actually got them running automatically on Travis here: https://github.com/Natim/MozillaPulse/pull/2
Assignee | ||
Comment 13•7 years ago
|
||
Ok I changed tox to use runtests.py rather than python setup.py test and I can see the error. Working on a patch.
Assignee | ||
Comment 14•7 years ago
|
||
Updates with mcote reviews.
Attachment #8869979 -
Attachment is obsolete: true
Attachment #8869979 -
Flags: feedback?(mathieu)
Attachment #8871659 -
Flags: review?(mcote)
Attachment #8871659 -
Flags: feedback?(mathieu)
Comment 15•7 years ago
|
||
I'm seeing Travis failures for the latest updates at https://github.com/Natim/MozillaPulse/pull/2. Can you fix those?
Flags: needinfo?(rhubscher)
Comment 16•7 years ago
|
||
I'm going to reassign this to natim to reflect the recent updates.
Assignee: mathieu → rhubscher
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•7 years ago
|
||
> I'm seeing Travis failures for the latest updates at https://github.com/Natim/MozillaPulse/pull/2. Can you fix those?
As we discussed earlier, the travis will be fixed in another issue. I need to run Pulse in Travis to be able to do so.
Since we are running tests locally with a Vagrant Image, I don't plan on fixing the Travis integration right now.
Also the travis config is not part of this patch.
Flags: needinfo?(rhubscher)
Assignee | ||
Comment 18•7 years ago
|
||
I tried porting the Vagrant puppet config to Travis here: https://github.com/Natim/MozillaPulse/pull/2/commits/979c60139ebe1df2370f2c211d048ed3a4359d9b But when I run test tests on travis they still timeout. If you know how to fix this, I would be glad to take your help.
Flags: needinfo?(mcote)
Assignee | ||
Comment 19•7 years ago
|
||
Apparently I need to pass the correct HOST IP rather than the Vagrant one which is the default.
Flags: needinfo?(mcote)
Assignee | ||
Comment 20•7 years ago
|
||
Ok tests are now fixed on travis. Do you want me to add the travis config to that patch?
Assignee | ||
Comment 21•7 years ago
|
||
Attachment #8871659 -
Attachment is obsolete: true
Attachment #8871659 -
Flags: review?(mcote)
Attachment #8871659 -
Flags: feedback?(mathieu)
Attachment #8872516 -
Flags: review?(mcote)
Updated•7 years ago
|
Attachment #8872516 -
Flags: feedback+
Comment 22•7 years ago
|
||
(In reply to Rémy Hubscher (:natim) from comment #17) > > I'm seeing Travis failures for the latest updates at https://github.com/Natim/MozillaPulse/pull/2. Can you fix those? > > As we discussed earlier, the travis will be fixed in another issue. Right sorry, I keep confusing this with PulseGuardian, which is on GitHub and has Travis support. Regardless, thanks for adding it! I wonder if we should make GitHub the canonical location of mozillapulse... Anyway for now I squashed your commits and pushed to hg: https://hg.mozilla.org/automation/mozillapulse/rev/91fd3fd624b8 Thanks very much!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Attachment #8872516 -
Flags: review?(mcote) → review+
Assignee | ||
Comment 23•7 years ago
|
||
> I wonder if we should make GitHub the canonical location of mozillapulse... If you can I would gladly vote for it. Yes :) It would make contributing to it much easier. > Thanks very much! Thank you for you kind reviews and taking the time.
Assignee | ||
Comment 24•7 years ago
|
||
Mark shall I migrate my fork it to the mozilla-services org?
Comment 25•7 years ago
|
||
> I wonder if we should make GitHub the canonical location of mozillapulse... I also think it would be easier to contribute (at least for a couple years ;)) > I squashed your commits and pushed to hg: https://hg.mozilla.org/automation/mozillapulse/rev/91fd3fd624b8 Excellent thanks! Any chance you could push it on Pypi?
Assignee | ||
Comment 26•7 years ago
|
||
Do you want to move it to Github before releasing?
Comment 27•7 years ago
|
||
Actually I think it's better if I release first, which I just did. :) Sure, moving it to mozilla-services would be fine. Maybe we should make a new pulse team? Might make sense for me to move PulseGuardian over too (it's currently in the generic mozilla org which is probably not a good place for it).
Assignee | ||
Comment 28•7 years ago
|
||
Do you plan on moving PulseGuardian over to the mozilla-services org? I guess if we have multiple repositories related to Pulse we could create an org, if we have only two maybe we don't need to do it right now. The Pulse org name is already taken, can I let you pick a name, and create the org if you wish or should we move both projects to mozilla-services?
Comment 29•7 years ago
|
||
I heard no objections to moving mozillapulse to GitHub, so I filed bug 1372731 for that.
You need to log in
before you can comment on or make changes to this bug.
Description
•