Make similarity client a stateful class

[alastair]

Description
When I was looking at this similarity client for some gaia testing, I noticed that you can't have 2 clients, each pointing to a different host.

This change lets you pass in a host into the constructor.

There are still some changes to make - we need to read the settings file to get the host where this client is created.
I also changed to using requests, but we should improve the methods to pass GET args as a dict instead of constructing them manually.

[alastair]

I've updated this to create an instance of the similarity client for both the normal server and the indexing server. This removes the requirement to have separate methods for add/save.

There are still some things that need to be improved:

• requests 'timeout' by default is infinite. We should set this to a specific limit, and then undo it for save if needed
• Still passing constructed parameters as the url, need to switch to params argument to requests.get. I'd like to be able to find a way to keep the tests the same (with a fully constructed url) in order to verify that we make this change correctly. I need to think about how these tests will fit together a bit better in order to do this
• move client file from similarity/client/init.py to similarity/client.py
• Fix conflicts

I still need to test with a real running similarity server

[ffont]

It looks good to me, and the missing changes you mention also look reasonable. About testing (once we pass query params as dict instead of building URL manually), I guess we just need to do assert_called_with and also check the params dict. Or if requests module has some method to "build URL" we could split the request in two parts, first build URL then call requests.get and we could mock requests.get. Doing a quick search I found you can do something like this:

requests.Request('GET', 'https://example.com/api', params=dict(param1='arg1',param2='arg2')).prepare().url 

Maybe this would work. Use this first then mock requests.get and do assert_called_with with the full URL.

[ffont]

cool, love that

[alastair]

yeah, I found the prepare().url syntax too. It's just a matter of seeing if I can write a test that contains the query parameter as "?foo=bar&etc", and then make the change to the code and see the tests stay the same. I'd prefer to use request's .get(baseurl, paramsdict) syntax, but maybe there's a middleground where I can construct the full url and pass it to .get() and then once the tests pass, move to the other syntax. I'll keep looking.

[alastair]

Hi @ffont, I can't remember why we never finished this work. Was it really just because I couldn't agree on how to construct the test URLs? If that's it, then I propose that we quickly fix this and merge it. I'll try and resolve the conflicts.

[ffont]

Yeah, I guess that was it! Also similar thing should be done for tag recommendation, although I want to refactor it as a celery task and running on py3 so maybe there's no need to make this update now.