1144417 - Treeherder client library should use requests, not httplib

Status: RESOLVED FIXED

(Tree Management Graveyard :: Treeherder: Client Libraries, defect)

Description

[William Lachance (:wlach)]

If we used requests, we could take advantage of connection pooling, among other things.

This is suitable as a good first bug for someone who is already somewhat familiar with python.

[ please only volunteer for this bug if you are willing to spend a few hours setting up treeherder-service and are willing to stick around to fix a few more things, it's just not worth our time or yours otherwise ]

Procedure: 

* Set up treeherder-service according to the instructions here: http://treeherder-service.readthedocs.org/en/latest/installation.html
* Modify treeherder-client to use the requests library instead of httplib: http://docs.python-requests.org/en/latest/
** You will need to modify in two places, first in this repo: https://github.com/mozilla/treeherder-client , then inside the in-tree copy in https://github.com/mozilla/treeherder-service/tree/master/vendor/thclient (just copy your work from one to the other). You will need to modify the client.py file in both. In the former, you will also need to modify setup.py to express the fact that the 'requests' library is now required.
* When you're satisfied with your work, test it out by ingesting a push and making sure things still work as expected (i.e. you see your local copy of treeherder filled out with results): http://treeherder-service.readthedocs.org/en/latest/installation.html#running-the-ingestion-tasks

Comment 1

[William Lachance (:wlach)]

Tapesh Mandal wants to take this on!

Comment 2

[Jonathan French (:jfrench)]

Just a note Tapesh had some setup issues with vagrant, so I gave him a quick ui-only bug to work on today (bug 1150938). He's setup and running web-server.

Comment 3

[Tapesh Mandal]

The proposed changes are as follows :-
 
In treeherder-service :-

vendor/thclient/client.py
*************************

-6 import httplib
+6 import requests

-732 conn=httplib.HTTPConnection(self.host.....
+732 conn=requests.get(self.host,auth=(self.oauth_key,self.oauth_secret),timeout=self.timeout)

same in 734

                          ---------------x---------------

In treeherder-client :-             

thclient/client.py
******************

same as treeherder-service/vendor/thclient/client.py


setup.py
********
comment out lines 11-14

-29 install_requires=deps
+29 install_requires=['requests']

                           -----------x-----------

things beyond my comprehension :-

i can't understand how to improve connection pooling..and if the above changes have got anything to do with that..also httplib is used in a few other files located at treeherder-service/vendor/requests/packages/urllib3/..mostly. Do we need to change them as well ?

if changes are correct kindly tell me how to test them or maybe if possible you could test them so that i don't mess up things :P


Best wishes :-)

tapesh

Comment 4

[William Lachance (:wlach)]

(In reply to Tapesh Mandal from comment #3)
> The proposed changes are as follows :-
> ...

Could you file pull requests for these and do a review/feedback request here as described:

http://ateam-bootcamp.readthedocs.org/en/latest/guide/development_process.html#git-and-github

It's easier to evaluate changes when they're done in this way, because you can see the context. What you propose makes sense though. 

> things beyond my comprehension :-
> 
> i can't understand how to improve connection pooling..and if the above
> changes have got anything to do with that..also httplib is used in a few
> other files located at
> treeherder-service/vendor/requests/packages/urllib3/..mostly. Do we need to
> change them as well ?

You don't need to do anything to implement/improve connection pooling. The requests/urllib3 library will do it automatically, if it's possible. You don't want to change the `vendor/requests` library, that's a static copy of the software you're using to fix this bug. ;)

> if changes are correct kindly tell me how to test them or maybe if possible
> you could test them so that i don't mess up things :P

See the last step in comment 0 for instructions on how to test. Basically, run the server and then this management command: 

./manage.py ingest_push mozilla-central tip

If there are no errors on the console, and you see colored boxes appearing in http://local.treeherder.mozilla.org, your patch works.

Comment 5

[Tapesh Mandal]

Hi,
    Am facing issues regarding viewing the local server..pasting links to 2 relevant files..

treeherder-service.conf -----> http://paste.ubuntu.com/10820389/

apache.pp -------------------> http://paste.ubuntu.com/10820390/

Hope these links are useful... :)

Please help..

Best,
Tapesh Mandal

Comment 6

[William Lachance (:wlach)]

This isn't enough for me(In reply to Tapesh Mandal from comment #5)
> Hi,
>     Am facing issues regarding viewing the local server..pasting links to 2
> relevant files..
> ...

So I'm afraid that isn't very helpful. :) If I'm going to help, you need to be very specific about what's not working and what you tried. Are you still having trouble resolving local.treeherder.mozilla.org? Or are you seeing some kind of other problem? If so, what are the exact symptoms? Feel free to bring this discussion to irc, sometimes it's easier to debug these things in real time.

Comment 7

[MikeLing]

Hi~ I'm interesting in this bug and contribute to A-team. But I don't have Mac OS and the system I use is Ubuntu 12.04. And I wonder if the Description in Comment 0 still work for me or I need some extra information. Actually I have some problem when I compile Mozilla source code. So I would like to work on this bug if I only need to clone source code and work on Treeherder.

Please tell how I can help and I promise to fix this bug if you could assign it to me.

Thank you :)

Comment 8

[Jonathan French (:jfrench)]

Hi Mike! Generally speaking contributing to Treeherder is platform independent, it doesn't matter which host OS you are running.

You can find additional links on Treeherder Contributing and Setup in the Project Wiki, and how to find us on IRC, in the User Story at the top of this bug.

I believe William's information in Comment 0 is still valid.

nb. we are in the midst of a Github repo unification (treeherder-service, treeherder-ui, in the coming weeks will merge and become just 'treeherder' on Github). The treeherder-service repo on Github has already been renamed to 'treeherder' as part of that, but all of the setup steps we provide should still be valid. So at present, you will need both 'treeherder' (the service) and 'treeherder-ui' (the frontend) for this bug. You don't need to compile Mozilla Firefox, if that was your reference above.

Let us know if you would still like the bug assigned to you and we will do so :)

Comment 9

[MikeLing]

Great! Sorry for the late reply because the annoying time zone. :)

Sure, I'm still interesting in this bug and I had fork it on github already.

Thank you for your trust and I will try my best to fix it :)

Comment 10

[MikeLing]

Oops,the link about thclient has became 404(https://github.com/mozilla/treeherder-service/tree/master/vendor/thclient). Could you offer me another one or tell me how to get that repo? 

Thank you:)

Comment 11

[MikeLing]

Hmm,some trouble in here when I type:

>(venv)vagrant@precise32:~/treeherder$ ./setup.py build_ext --inplace

Traceback (most recent call last):
  File "./setup.py", line 8, in <module>
    from Cython.Build import cythonize
ImportError: No module named Cython.Build

Could you tell me what should I do for this?

Comment 12

[MikeLing]

Hmm, I fix the No Cython error but a new problem comes out when I type:

> ./manage.py init_master_db

And it tells me django.core is missing. I use "pip install django" to fix that, but when I type init_master_db again:

> from django.apps import apps
> ImportError: No module named apps

I  wonder if there is something wrong with my vagrant installation. Please tell me what should I do next. Thanks a lot:)

Comment 13

[Jonathan French (:jfrench)]

(In reply to MikeLing from comment #9)
> Great! Sorry for the late reply because the annoying time zone. :)
> Thank you for your trust and I will try my best to fix it :)

Sure, no problems Mike, I will assign you now. Please take your time with each setup step, there is no rush for the bug fix on our end.

If there's any concerns you have that you may have omitted a step in the installation, you might want to do a vagrant destroy, and then a new vagrant up, and start the installation steps again to be sure.

Your excerpted django error above does not contain a (venv) command prompt. Perhaps you weren't in the venv when you issued that command?

Just an fyi, we have Treeherder mentors in GMT timezone, some in GMT+5, and others in GMT+8, and are usually available on IRC during typical work hours for each.

With the repo-merge work the last several weeks, it's a possibility it could be a configuration issue in our repo, not on your end. But in any case you can contact us in channel if after repeating the installation process you encounter the same error.

Thank you for helping out with Treeherder!

Comment 14

[Ed Morley [:emorley]]

Regarding the broken link - treeherder now does not have an in-repo vendor directory - and instead uses the treeherder-client version from Pypi. As such, in the comment 0 steps, you need to fix the issue first in treeherder-client (https://github.com/mozilla/treeherder-client) then ask wlach to release a new treeherder-client version, which can then be pointed at by:
https://github.com/mozilla/treeherder/blob/3b0469dbc5c10797866b18ef3bad665ca7042c92/requirements/common.txt#L75
(the hash will also need updating, just use the value peep will give you when it errors out, either locally or when the tests fail when pushing to Travis).

Comment 15

[William Lachance (:wlach)]

Hey Mike, just a note that I need this bug fixed in the next day or so for another bug I'm planning to work on, so I might need to take this from you if you're still stuck by mid-week. :) There should be other treeherder stuff for you to work on if interested of course.

Re: Vagrant troubles, :jfrench's advice is good. If you're still having trouble bringing things up in a state where they work, please post a log of what the provisioning step looks like in your console. That is really the best way of troubleshooting what's going on (as opposed to trying to figure out what's broken after the provisioning failed).

Comment 16

[MikeLing]

(In reply to William Lachance (:wlach) from comment #15)

Please be free to do that because I think I can't promise solve it soon(my time zone is UTC-8, so I must wait a long time until someone could help me how to solve problem)
I will be very appreciate it if you can assign another bug about treeherder.:)

Comment 17

[MikeLing]

(In reply to Jonathan French (:jfrench) from comment #13)

Thank you for your help!:)

Hmm, you are right, no Python virtual environment in there is exactly where the problem is. But after I type vagrant up and vagrant ssh, the vagrant environment up but still no virtual environment. I think I should try it again from begin.

Comment 18

[Jonathan French (:jfrench)]

(In reply to MikeLing from comment #17)
> (In reply to Jonathan French (:jfrench) from comment #13)
> after I type vagrant up and vagrant ssh, the vagrant environment up but still no virtual environment.

Hi Mike, just check to be sure you are in your /treeherder directory (ie. where the Vagrant file is) when you are typing `vagrant ssh`. The docs imply it, but if that was your problem I will make the docs more clear.

Comment 19

[MikeLing]

(In reply to Jonathan French (:jfrench) from comment #18)

Great! I fix that and seems like everything works fine(output like):

> [2015-04-28 02:59:08,543: WARNING/MainProcess] celery@local ready.

I think I can start to fix this issue and commit first patch ASAP :)

Comment 20

[MikeLing]

(In reply to Ed Morley [:emorley] from comment #14)

Hmm, is that means I need to use requests library instead of httplib in client.py(https://github.com/mozilla/treeherder-client/blob/master/thclient/client.py), then ask wlach for next setp?

Comment 21

[Ed Morley [:emorley]]

Yeah that's correct :-)

So you'll also need to update https://github.com/mozilla/treeherder-client/blob/master/setup.py#L22

Comment 22

[MikeLing]

I think I found the exact place where I should start to work with(https://github.com/mozilla/treeherder-client/blob/master/thclient/client.py#L624-L743). Please correct me if I was wrong. 
Thank you very much :)

Comment 23

[Mauro Doglio [:mdoglio]]

To narrow the scope of your changes:
https://github.com/mozilla/treeherder-client/blob/59127e22f7337559b00b8912c05abe132479f066/thclient/client.py#L727-L735
requests has a much more concise api, it could be a 2 lines patch :-)
Please remember to update the requirements too:
https://github.com/mozilla/treeherder-client/blob/59127e22f7337559b00b8912c05abe132479f066/setup.py#L22

Comment 24

[MikeLing]

Attachment: fix bug 1144417

This is my first time to use requests even httplib, so I'm not sure if this pull request will be ok.

Please let me know if anything should be change! Thank you very much :)

Comment 25

[MikeLing]

Attachment: Screenshot from 2015-04-28 19:32:00.png

I type commend " celery -A treeherder worker -B " after I change the code. I don't know if it can tell me everything works fine. Please tell me if it's not.

Thank you!

Comment 26

[William Lachance (:wlach)]

Comment on attachment 8598577 [details] [review]
fix bug 1144417

I'm afraid this patch won't work -- there are still some lines of code in the file that assume the old httplib api. Do you know much about HTTP, oauth, etc.? You really need to understand those concepts to fix this bug. I'd advise looking closely at the requests documentation http://docs.python-requests.org/en/latest/ and use google for information on the http concepts if you're not yet familiar with them.

For testing, my advice would to be just ingest a single push per the instructions here: 

http://treeherder-service.readthedocs.org/en/latest/installation.html#running-the-ingestion-tasks

./manage.py ingest_push mozilla-central tip 

should do the trick. I wouldn't bother with celery for this initial test, though for what it's worth it looks like you were starting it correctly.

Comment 27

[Cameron Dawson [:camd]]

Ends up this change has some complexities to it.  It will also require changes in the ``treeherder`` repo, because of the differences in ``response`` objects between the two.

Comment 28

[Cameron Dawson [:camd]]

Mike- thanks for taking a crack at this.  But I'm going to take this one over.  It's a little deeper than we'd thought.

Comment 29

[Cameron Dawson [:camd]]

Attachment: client pr

Comment 30

[Cameron Dawson [:camd]]

Attachment: service pr

Comment 31

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder-client

https://github.com/mozilla/treeherder-client/commit/3b67692e5e0a36ec8d3e3df40dc084f82e60113d
Bug 1144417 - Treeherder client use requests

Comment 32

[William Lachance (:wlach)]

Comment on attachment 8599611 [details] [review]
service pr

Looks good except I think you need to change the expected SHA for the treeherder-client inside the requirements file. r+ with that addressed.

Comment 33

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/b0a0ea7a8c71e5e887c2e0b450aff270bb83c875
Bug 1144417 - use treeherder-client 1.2 with requests

Comment 34

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/437a3c379a6a1ec545e348763dd141e67af64217
Bug 1144417 - Treeherder client use requests

Comment 35

[Ed Morley [:emorley]]

We no longer specify a timeout, and the default requests timeout is None - which is what I believe is causing the log parser hangs on stage:
https://github.com/mozilla/treeherder/commit/437a3c379a6a1ec545e348763dd141e67af64217#diff-8f20c700b32fc2a7be610d1089eb9eb4L744

Comment 36

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/efa98c7528826edbc8bf19b131bbfedde6d315be
Revert most of 437a3c3 (the switch to requests in treeherder-client)

This reverts bug 1144417 (Treeherder-client using requests) apart from
the version bump hunk - for causing bug 1160856.

https://github.com/mozilla/treeherder/commit/c9f837f747b792714e02ca3cdd7d6e078ece97d1
Revert part of b0a0ea7 (bug 1144417) since th-client isn't using requests

Since the switch to requests has been backed out (437a3c3), we also need
to undo the service-side change that was made separately to support it.

Comment 37

[Cameron Dawson [:camd]]

Attachment: do-over PR

Now that the client is in the main repo, we only need 1 PR.

Comment 38

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/19e333dd0d04c1bbeadf5b93dd9b43b29cc16663
Bug 1144417 - Use requests rather than httplib in treeherder-client

Now implemented using the ``timeout`` param.  Same value as before with httplib.