Skip to content
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

Cleanup API: Refactor request on-complete transition #80

Merged
merged 96 commits into from
Apr 27, 2021

Conversation

jerinphilip
Copy link
Contributor

@jerinphilip jerinphilip commented Mar 31, 2021

(WIP Rework of the Request -> Response transition process)

Over here we're not in the small PR making business, we're in moving source from complete to more complete stages business, leaving intermediates stable.

To simplify diff - merge #85 which provides essential bugfixes for what would previously be undefined behaviour, followed by #79 which requires the bugfixes and eliminates the usage of history.

(Edit: This is untangled now)

Incorporating both the above, what this PR does is abstract Request -> Response transition using ResponseBuilder.

  • Request calls ResponseBuilder on completion with histories, and this triggers the construction of Response.
  • ResponseBuilder is also tasked with making certain objects optional like Alignment and QualityScores.
  • To test ResponseBuilder, it has to be wired to Service and all the way to command-line apps, requiring edits in these places.
  • To be wired to Service in a feature-complete setting, we require the notion of ResponseOptions (in ResponseBuilder), which is analogous to TranslationRequest.
  • So to move from complete sources to complete sources all 4 of the above has to be done.

There are the last few changes that can be moved to a separate collapse PR. behind this, will do by git-magic if required. The structures operated are documented as edits happen, so this PR also includes documentation changes.

We have big PR, yet again.

Jerin Philip added 6 commits March 31, 2021 14:45
Conflicts:
   src/translator/response.cpp -> Bringing to sync with ByteArray updates
   src/translator/response.h -> Bringing to sync with ByteArray updates
We however have Histories in constructor, which we will remove out of the way
soon.
@jerinphilip jerinphilip marked this pull request as draft March 31, 2021 20:37
@jerinphilip jerinphilip changed the title Refactor the transition on completion of a request to cleanup API Cleanup API: Refactor the transition on completion of a request Mar 31, 2021
@jerinphilip jerinphilip changed the title Cleanup API: Refactor the transition on completion of a request Cleanup API: Refactor request on-complete translation Mar 31, 2021
@jerinphilip jerinphilip changed the title Cleanup API: Refactor request on-complete translation Cleanup API: Refactor request on-complete transition Mar 31, 2021
@kpu
Copy link
Member

kpu commented Apr 26, 2021

One concern about friends: can we mock out the system? It can be useful for users to be able to make their own response object for testing purposes.

app/service-cli-bytearray.cpp Outdated Show resolved Hide resolved

QualityScoreType qualityScoreType{QualityScoreType::FREE};
ConcatStrategy concatStrategy{ConcatStrategy::FAITHFUL};
};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kpu Removed the Options in favour of a cpp struct, if something needs to stay/go among members or requires rename etc please let know.

We're looking at collapse of TranslationRequest and ResponseOptions. I'm deferring collapse here to avoid editing bindings.

@jerinphilip
Copy link
Contributor Author

Please treat this as an explicit request to expedite merge @abhi-agg, @XapaJIaMnu, @kpu.

  1. What was previously on the Options framework (ResponseOptions) is now a struct, per @kpu's review.
  2. The changes @XapaJIaMnu wants for MSVC compile (Does not compile on windows megathread #105 (comment)) are the last few commits. The sources are moved around between files, so make sense to apply those in the cleanup commit as well.

Comment on lines +19 to +23
FREE,

/// An expensive quality-score that runs additional computations to determine
/// quality of an output.
EXPENSIVE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding that we probably would want to expose these scores at different text granularity as well. e.g. Word level, Sentence level. But we can update this enum later once we do more experimentation 👍

Copy link
Collaborator

@XapaJIaMnu XapaJIaMnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@abhi-agg abhi-agg self-requested a review April 27, 2021 13:41
Copy link
Contributor

@abhi-agg abhi-agg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me as well 👍

@jerinphilip jerinphilip merged commit fa2003e into main Apr 27, 2021
@jerinphilip jerinphilip deleted the jp/response-builder branch April 27, 2021 14:56
@jerinphilip jerinphilip mentioned this pull request Apr 29, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: marian Changes affecting marian-dev component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Callback on Request completion to construct Response Remove Histories from Response
4 participants