Closed Bug 1294230 Opened 8 years ago Closed 7 years ago

mozillapulse bindings are incompatible with python 3

Categories

(Webtools :: Pulse, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: amiyaguchi, Assigned: rhubscher)

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Attached patch Port to Python 3 (obsolete) — Splinter Review
Hi Mark!
I'm interested in porting MozillaPulse to Python3.

I submitted a patch, but what is your preferred workflow?
Thanks!
Flags: needinfo?(mcote)
Assignee: nobody → mathieu
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 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-
Oh please set me as reviewer when you post a new patch. :) Thanks!
Attached patch port_to_python3.patch (obsolete) — Splinter Review
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)
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?
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 :).
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 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-
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
Ok I changed tox to use runtests.py rather than python setup.py test and I can see the error. Working on a patch.
Attached patch port_to_python3.patch (obsolete) — Splinter Review
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)
I'm seeing Travis failures for the latest updates at https://github.com/Natim/MozillaPulse/pull/2.  Can you fix those?
Flags: needinfo?(rhubscher)
I'm going to reassign this to natim to reflect the recent updates.
Assignee: mathieu → rhubscher
Status: NEW → ASSIGNED
> 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)
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)
Apparently I need to pass the correct HOST IP rather than the Vagrant one which is the default.
Flags: needinfo?(mcote)
Ok tests are now fixed on travis. Do you want me to add the travis config to that patch?
Attachment #8871659 - Attachment is obsolete: true
Attachment #8871659 - Flags: review?(mcote)
Attachment #8871659 - Flags: feedback?(mathieu)
Attachment #8872516 - Flags: review?(mcote)
(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
Attachment #8872516 - Flags: review?(mcote) → review+
> 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.
Mark shall I migrate my fork it to the mozilla-services org?
> 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?
Do you want to move it to Github before releasing?
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).
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?
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.

Attachment

General

Created:
Updated:
Size: