Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Unified api draft #1
Unified api draft #1
Changes from all commits
1a1252f
715abe5
e954e45
1f131bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Still has the same bug: should be one
std::future<std::vector<TranslationResult>>
.Also
std::vector<std::string> &
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.
Comment that the
std::string
will be stolen and moved to the result.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.
Having separate std::future for each text might give some performance benefit. Surely, in current implementation it might not give us sufficient gain but a separate future for each text entry makes sense as the api should be agnostic of the implementation details of marian.
Didn't we agree in previous review that the api should have the ownership of the
texts
in this API call? Passing a reference leaves a possibility of thetexts
to be modified by consumers of the API.This comment would not make sense if I am passing a copy of
texts
and not referenceThere 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 efficient semantic is that
texts
moves totranslate
and can do what it wants with it (and will move it toTranslationRequest
.I think the most C++ way of doing this is
and the caller does
translate(std::move(texts), request)
You were the one saying we don't want to copy the input too much...
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 you want to design a correct API for incremental returns that would do sentences at a time, you can do that later. It doesn't look like
std::vector<std::future<TranslationResult>>
. It looks like this span translates as this span in undefined order.There is no efficient toolkit that would take a batch of requests and return them in the original order. It will require more work for you and for us to implement a less efficient API that nobody will support.
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 don't want the caller to retain write access to those
std::string
. They are not shared. They are transferred. I realize I can't prevent writing to them. Cough on astd::string
and the base pointer changes and all thestd::string_view
are invalid.Which is why:
And we just define that the passed vector is clear at the end of the
translate
call.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 still very much like the idea of using shared pointers. How about something along these lines:
That way you get what you want (immutability) and I get what I want (shared pointers to make sure the strings are there as long as I need them).
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.
Dude they're moving into the response. The
string_view
classes depend on them. They will exist. I hate unnecessary malloc calls.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.
How many more malloc calls does my proposal involve?
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.
For now, I will change the API
std::vector<std::future<TranslationRequest>> translate(std::vector<std::string> texts, TranslationRequest request);
to
std::future<std::vector<TranslationRequest>> translate(std::vector<std::string> && texts, TranslationRequest request);
It is a first version of the API which. In any case, changing the
texts
to support a move paradigm&& texts
or making it a shared/unique pointer is never a big problem.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.
Again, since we are returning
string_view
returning the underlyingstd::string
is essentially required. There should be no option here.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.
Will remove it.
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 we're going to abstract this so much, then we might as well derive the vocab paths from the model path.
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.
What type of alignments do we want? Really soft alignments? No hard alignments?
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.
We get soft alignments natively. As to whether the Mozilla people want those hardened in C++, I'll leave that up to them.
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.
In March I was told that it makes more sense to return hard alignments. I implemented both in the REST API, so I don't really care. I return what I'm being asked for. Soft alignments just add a lot of JSON clutter. One thing to keep in mind is that responses through Native Messaging are capped at 1MB per response message (browser to co-app: 4GB).
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 do not really see a need for this class. A quality estimate associates target text spans with a number, so a
std::vector<std::pair<TextSpan,float>>
could be a direct data member of the TranslationResult class. We could have two such vectors, one for word-level estimates, one for sentence-level estimates.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.
Agreed having a class is overkill.
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.
Is this really extendable? Imagine if we agree on supporting document level QE scores in future. Would we add one more vector there for document level QE scores? I believe this design is not extendable and this is the reason for a separate class.
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.
You're either adding to an enum or adding a member variable. I don't see how adding a member variable is any less extensible; we're not aiming for ABI compatibility here. And @ugermann 's design has the advantage that multiple things can be returned in the same query---including document and sentence-level QE.
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 would be adding an enum as well as adding a member variable for the solution proposed by @ugermann
Didn't we agree on that there is no use case of returning QE scores for multiple granularities in the same request? I fail to understand how this use case came into picture again when you suggested me not to implement unnecessary stuff. And by the way, extending this API to this use case only require changing
QualityScore
tovector<QualityScore>
in TranslationResult.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.
All this matching indices is overcomplicated compared to just having a std::vector of a struct, 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.
Also, efficiency nit that this will require tons of memory allocation with all these std::vector running around.
Better: