-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added Support for handling multiple 'q' parameters #105
Conversation
Pull Request Test Coverage Report for Build 886
💛 - Coveralls |
One of the problems which were there in the previous PR was how the response was structured as I was sending the whole local response in the |
When you do |
Finally, I was able to find the equivalent which you were referring to. I found out that tornado provides a similar functionality in the form of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good! Some minor remarks.
apertium_apy/handlers/translate.py
Outdated
@@ -157,26 +157,38 @@ def translate_and_respond(self, pair, pipeline, to_translate, mark_unknown, nosp | |||
before = self.log_before_translation() | |||
translated = yield pipeline.translate(to_translate, nosplit, deformat, reformat) | |||
self.log_after_translation(before, len(to_translate)) | |||
self.send_response({ | |||
self.clean_pairs() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that we will clean pairs for each individual q. Is that what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I don't clearly get what clean pairs mean. Can you please explain this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It restarts pipes that are highly contentious and old. Take a look at the code path and it should be relatively clear. Honestly, I think translate_and_respond
can be removed and its functionality collapsed into get
so that cleaning pairs and logging can be done once instead of many times. Right now, we're doing the same thing a bunch of times inside the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the case, then I think clean pairs can be done in get
only. Also, since gen.multi_future
guarantees resolving futures in parallel, isn't that more efficient than collapsing translate_and_respond
into get
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're not mutually exclusive. Don't use the yield
from translate_and_respond
and you get a future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what do you suggest we do about this? and should I put the clean pairs in get
only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collapse would be good and yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While collapsing should I use yield
to get the translated text and create a local response in the same loop or use gen.multi_future
and create the local response in a different loop before creating the global response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter since then you will block on all the translations rather than a single one.
apertium_apy/handlers/translate.py
Outdated
reformat=reformat) | ||
query_list = self.get_arguments('q') | ||
langpair = self.get_argument('langpair') | ||
list_of_futures = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
list_of_futures
-> translations
or translation_futures
would be clearer
apertium_apy/handlers/translate.py
Outdated
langpair = self.get_argument('langpair') | ||
list_of_futures = [] | ||
for query in query_list: | ||
pair = self.get_pair_or_error(langpair, len(query)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line and the next three can go outside the loop, yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of keeping it outside the loop but then I saw that we had to pass the length of the query string to be translated which is used for some logging purposes. Not sure what that is used for though. And other lines after that are dependent on this line, that is why I had to put it inside the loop instead of keeping it outside. What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending the sum of the lengths should be OK.
Also, please add a test that verifies multiple q params works correctly. It should just be 3-5 LOC. |
There is an Incompatible type assignment error which is coming on running |
What exactly is the type error? Also, tests should go in the same PR so I can verify that they pass. |
By type error, I mean that in line 181, I am assigning a dict to a variable which contains a list on which an error is raised by |
tests/test.py
Outdated
response = self.fetch_translation(['welcome', 'respect', 'serve'], 'eng|spa') | ||
translations = [] | ||
for translation_response in response['responseData']: | ||
translations.append(translation_response['responseData']['translatedText']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a list comprehension or map
, depending on your preference.
Yeah, I think mypy is being a bit trigger happy here. I suggest just using an inline ternary. i.e.
I'm not a huge fan of variables changing type anyway. |
apertium_apy/handlers/translate.py
Outdated
translation_futures.append(pipeline.translate(query, nosplit=False, deformat=deformat, reformat=reformat)) | ||
self.log_after_translation(before, len(query)) | ||
|
||
translations = yield gen.multi_future(translation_futures) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think L170 onwards needs to be indented.
if pair is not None:
return
is also fine. I like early returns but it's really up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clear something you mean
if pair is None:
return
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah. My bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine, I'll do it right now.
@unhammer see any issues? |
What about subclass users of |
Good point, missed that. Looks like |
Although, I get that we need to replicate the functionality in |
Create a method in the base translation handler called |
apertium_apy/handlers/base.py
Outdated
@@ -60,6 +60,10 @@ class BaseHandler(tornado.web.RequestHandler): | |||
def initialize(self): | |||
self.callback = self.get_argument('callback', default=None) | |||
|
|||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not here, in the base translation handler. Looks good otherwise
b36ac97
to
51fb9c5
Compare
Currently, APy doesn't support multiple q's as parameters when given for translation. I have resolved this by storing multiple responses(local) first and then sending all at once in a global response. Each parameter q is iteratively translated by a given langpair and an error is raised if at least one of the translation fails.
refs #86