Move ranker_model_loader to a new component.

Move ranker_model_loader to a new component.

This change should not incur any behavior change. It moves the RankerModelLoader code from components/translate to a new component. The ModelLoader (and other parts of Ranker) will be re-used by contextual_search (and probably other components eventually).

The plan for the machine_intelligence component is that it will hold the code for machine intelligence features in Chrome. In the short term, we plan to put the Ranker code here (go/chromeranker), including the ModelLoader, example logging and model inference.

The ModelLoader is the first part to be moved out of Translate, since it is already ready for re-use by other components (e.g. Contextual Search). Other parts will need to be refactored first before we move them out of the Translate component.



BUG=731845
Review-Url: https://codereview.chromium.org/2925733002
Cr-Commit-Position: refs/heads/master@{#482754}
Committed: https://chromium.googlesource.com/chromium/src/+/97b0ab02964dac76c919c24cac8b50ad1141cd24

[Roger McFarlane (Chromium), 2017-06-06 09:48:30]

rogerm@chromium.org changed reviewers:
+ rogerm@chromium.org

[Roger McFarlane (Chromium), 2017-06-06 09:48:31]

Nice.

I think I prefer intelligence to ranker as a component/namespace identifier.
Ranker is both less general and more ambiguous... go figure.

:)

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
File components/ranker/ranker_model_loader_unittest.cc (left):

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
components/ranker/ranker_model_loader_unittest.cc:139: locale_ =
TranslateDownloadManager::GetInstance()->application_locale();
ah, yes, I remember what this was for... it just made sure that the locale was
restored at the end of the test. Groby had suggested I specifically test in a
locale other than the default en-US.

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
File components/ranker/ranker_model_loader_unittest.cc (right):

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
components/ranker/ranker_model_loader_unittest.cc:95: // FIXME
?

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
components/ranker/ranker_model_loader_unittest.cc:136: //    
request_context_.get());
I would expect this to abort download if not set.

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_ur...
File components/ranker/ranker_url_fetcher.cc (right):

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_ur...
components/ranker/ranker_url_fetcher.cc:81: //
fetcher_->SetRequestContext(request_context_getter.get());
yup... this is a problem. More specifically, the start below fails with a real
fetcher_ if this set doesn't happen. It *might* work with a fake url fetcher,
but that's not guaranteed.

[hamelphi, 2017-06-09 18:49:15]

Changed the component name to machine_intelligence.

Still a few FIXMEs to take care of, but most of the work is done.

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
File components/ranker/ranker_model_loader_unittest.cc (left):

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
components/ranker/ranker_model_loader_unittest.cc:139: locale_ =
TranslateDownloadManager::GetInstance()->application_locale();
On 2017/06/06 09:48:30, Roger McFarlane (Chromium) wrote:
> ah, yes, I remember what this was for... it just made sure that the locale was
> restored at the end of the test. Groby had suggested I specifically test in a
> locale other than the default en-US.

Not relevant anymore.

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
File components/ranker/ranker_model_loader_unittest.cc (right):

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
components/ranker/ranker_model_loader_unittest.cc:95: // FIXME
On 2017/06/06 09:48:30, Roger McFarlane (Chromium) wrote:
> ?

Done.

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_mo...
components/ranker/ranker_model_loader_unittest.cc:136: //    
request_context_.get());
On 2017/06/06 09:48:31, Roger McFarlane (Chromium) wrote:
> I would expect this to abort download if not set.

Not relevant anymore.

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_ur...
File components/ranker/ranker_url_fetcher.cc (right):

https://codereview.chromium.org/2925733002/diff/1/components/ranker/ranker_ur...
components/ranker/ranker_url_fetcher.cc:81: //
fetcher_->SetRequestContext(request_context_getter.get());
On 2017/06/06 09:48:31, Roger McFarlane (Chromium) wrote:
> yup... this is a problem. More specifically, the start below fails with a real
> fetcher_ if this set doesn't happen. It *might* work with a fake url fetcher,
> but that's not guaranteed.

Fixed.

[hamelphi, 2017-06-09 18:49:37]

Description was changed from

==========
Move ranker_model_loader to its own component.

BUG=
==========

to

==========
Move ranker_model_loader to its own component.

BUG=
==========

[hamelphi, 2017-06-09 19:43:08]

Description was changed from

==========
Move ranker_model_loader to its own component.

BUG=
==========

to

==========
Move ranker_model_loader to its own component.

BUG=731845
==========

[hamelphi, 2017-06-09 19:43:37]

Ready for full review. Roger PTAL.

[Roger McFarlane (Chromium), 2017-06-14 15:59:43]

I think the url request annotation in translate_url_fetcher.cc needs to be
updated to no longer reference the model download (since it's no longer used for
that).

Please make clear in the CL description that this change just creates a new
component and moves/copies files into it.

Otherwise, LGTM.

[hamelphi, 2017-06-15 18:33:59]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 18:34:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 18:44:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 18:44:23]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hamelphi, 2017-06-15 18:45:45]

Description was changed from

==========
Move ranker_model_loader to its own component.

BUG=731845
==========

to

==========
Move ranker_model_loader to its own component.

This change should not incur any behavior change. It moves the RankerModelLoader
code to a new component. 

The plan for the  machine_intelligence is that it will hold the code for general
machine intelligence features in Chrome. In the short term, We want to move most
of the Ranker code here, including the ModelLoader, example logging and model
inference.

The ModelLoader is the first part to be moved, since it is already ready for
re-use by other components (e.g. Contextual Search). Other parts will need to be
refactored first before we move them out of the Translate component.



BUG=731845
==========

[hamelphi, 2017-06-15 18:57:30]

Description was changed from

==========
Move ranker_model_loader to its own component.

This change should not incur any behavior change. It moves the RankerModelLoader
code to a new component. 

The plan for the  machine_intelligence is that it will hold the code for general
machine intelligence features in Chrome. In the short term, We want to move most
of the Ranker code here, including the ModelLoader, example logging and model
inference.

The ModelLoader is the first part to be moved, since it is already ready for
re-use by other components (e.g. Contextual Search). Other parts will need to be
refactored first before we move them out of the Translate component.



BUG=731845
==========

to

==========
Move ranker_model_loader to a new component.

This change should not incur any behavior change. It moves the RankerModelLoader
code from components/translate to a new component. The ModelLoader (and other
parts of Ranker) will be re-used by contextual_search.

The plan for the machine_intelligence component is that it will hold the code
for general machine intelligence features in Chrome. In the short term, we plan
to put the Ranker code here, including the ModelLoader, example logging and
model inference.

The ModelLoader is the first part to be moved out of Translate, since it is
already ready for re-use by other components (e.g. Contextual Search). Other
parts will need to be refactored first before we move them out of the Translate
component.



BUG=731845
==========

[hamelphi, 2017-06-15 19:04:06]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 19:04:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 20:06:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 20:06:11]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hamelphi, 2017-06-15 20:09:17]

The CQ bit was checked by hamelphi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 20:09:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hamelphi, 2017-06-15 20:11:00]

Description was changed from

==========
Move ranker_model_loader to a new component.

This change should not incur any behavior change. It moves the RankerModelLoader
code from components/translate to a new component. The ModelLoader (and other
parts of Ranker) will be re-used by contextual_search.

The plan for the machine_intelligence component is that it will hold the code
for general machine intelligence features in Chrome. In the short term, we plan
to put the Ranker code here, including the ModelLoader, example logging and
model inference.

The ModelLoader is the first part to be moved out of Translate, since it is
already ready for re-use by other components (e.g. Contextual Search). Other
parts will need to be refactored first before we move them out of the Translate
component.



BUG=731845
==========

to

==========
Move ranker_model_loader to a new component.

This change should not incur any behavior change. It moves the RankerModelLoader
code from components/translate to a new component. The ModelLoader (and other
parts of Ranker) will be re-used by contextual_search (and probably other
components eventually).

The plan for the machine_intelligence component is that it will hold the code
for machine intelligence features in Chrome. In the short term, we plan to put
the Ranker code here (go/chromeranker), including the ModelLoader, example
logging and model inference.

The ModelLoader is the first part to be moved out of Translate, since it is
already ready for re-use by other components (e.g. Contextual Search). Other
parts will need to be refactored first before we move them out of the Translate
component.



BUG=731845
==========

[hamelphi, 2017-06-15 20:11:41]

hamelphi@chromium.org changed reviewers:
+ groby@chromium.org, sdefresne@chromium.org

[commit-bot: I haz the power, 2017-06-15 20:48:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 20:48:55]

Dry run: This issue passed the CQ dry run.

[groby-ooo-7-16, 2017-06-15 21:05:10]

lgtm

[sdefresne, 2017-06-23 16:18:45]

lgtm with comments

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
File components/machine_intelligence/ranker_model.h (right):

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
components/machine_intelligence/ranker_model.h:43: }  // namesapce ranker
namesapce → namespace
ranker → machine_intelligence

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
components/machine_intelligence/ranker_model.h:45: #endif  //
COMPONENTS_RANKERRANKER_MODEL_H_
COMPONENTS_RANKERRANKER_MODEL_H_ →
COMPONENTS_MACHINE_INTELLIGENCE_RANKER_MODEL_H_

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
File components/machine_intelligence/ranker_url_fetcher.h (right):

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
components/machine_intelligence/ranker_url_fetcher.h:32: explicit
RankerURLFetcher();
Remove "explicit" (this is only required for constructor taking a single
argument).

[hamelphi, 2017-06-27 18:15:09]

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
File components/machine_intelligence/ranker_model.h (right):

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
components/machine_intelligence/ranker_model.h:43: }  // namesapce ranker
On 2017/06/23 16:18:44, sdefresne wrote:
> namesapce → namespace
> ranker → machine_intelligence

Done.

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
components/machine_intelligence/ranker_model.h:45: #endif  //
COMPONENTS_RANKERRANKER_MODEL_H_
On 2017/06/23 16:18:44, sdefresne wrote:
> COMPONENTS_RANKERRANKER_MODEL_H_ →
> COMPONENTS_MACHINE_INTELLIGENCE_RANKER_MODEL_H_

Done.

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
File components/machine_intelligence/ranker_url_fetcher.h (right):

https://codereview.chromium.org/2925733002/diff/120001/components/machine_int...
components/machine_intelligence/ranker_url_fetcher.h:32: explicit
RankerURLFetcher();
On 2017/06/23 16:18:44, sdefresne wrote:
> Remove "explicit" (this is only required for constructor taking a single
> argument).

Done.

[hamelphi, 2017-06-27 18:18:19]

The CQ bit was checked by hamelphi@chromium.org

[hamelphi, 2017-06-27 18:18:19]

The patchset sent to the CQ was uploaded after l-g-t-m from rogerm@chromium.org,
sdefresne@chromium.org, groby@chromium.org
Link to the patchset: https://codereview.chromium.org/2925733002/#ps140001
(title: "Adressing sdefresne's comments.")

[commit-bot: I haz the power, 2017-06-27 18:18:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 18:27:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 18:27:18]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hamelphi, 2017-06-27 18:29:37]

hamelphi@chromium.org changed reviewers:
+ asvitkine@chromium.org

[hamelphi, 2017-06-27 18:30:21]

On 2017/06/27 18:27:18, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Adding asvitkine@ as reviewer for OWNER approval for the change in
histograms.xml

[hamelphi, 2017-06-27 19:06:39]

hamelphi@chromium.org changed reviewers:
+ mmenke@chromium.org

[hamelphi, 2017-06-27 19:10:02]

On 2017/06/27 18:30:21, hamelphi wrote:
> On 2017/06/27 18:27:18, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Adding asvitkine@ as reviewer for OWNER approval for the change in
> histograms.xml

Adding mmenke@  as reviewer for approval for '+net' in
components/machine_intelligence/DEPS file.

This follows from the presubmit warning:

You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
    '+net',
Suggested missing target path OWNERS:
    mmenke@chromium.org

[mmenke, 2017-06-27 19:44:32]

Rubberstamp LGTM, since this is a move.

[Alexei Svitkine (slow), 2017-06-27 20:48:04]

histograms.xml lgtm

[hamelphi, 2017-06-27 21:35:50]

The CQ bit was checked by hamelphi@chromium.org

[commit-bot: I haz the power, 2017-06-27 21:36:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 21:42:03]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498599350409520,
"parent_rev": "c9ae6a1a1214f69cb9ff0180468c669e82a380e6", "commit_rev":
"97b0ab02964dac76c919c24cac8b50ad1141cd24"}

[commit-bot: I haz the power, 2017-06-27 21:42:21]

Description was changed from

==========
Move ranker_model_loader to a new component.

This change should not incur any behavior change. It moves the RankerModelLoader
code from components/translate to a new component. The ModelLoader (and other
parts of Ranker) will be re-used by contextual_search (and probably other
components eventually).

The plan for the machine_intelligence component is that it will hold the code
for machine intelligence features in Chrome. In the short term, we plan to put
the Ranker code here (go/chromeranker), including the ModelLoader, example
logging and model inference.

The ModelLoader is the first part to be moved out of Translate, since it is
already ready for re-use by other components (e.g. Contextual Search). Other
parts will need to be refactored first before we move them out of the Translate
component.



BUG=731845
==========

to

==========
Move ranker_model_loader to a new component.

This change should not incur any behavior change. It moves the RankerModelLoader
code from components/translate to a new component. The ModelLoader (and other
parts of Ranker) will be re-used by contextual_search (and probably other
components eventually).

The plan for the machine_intelligence component is that it will hold the code
for machine intelligence features in Chrome. In the short term, we plan to put
the Ranker code here (go/chromeranker), including the ModelLoader, example
logging and model inference.

The ModelLoader is the first part to be moved out of Translate, since it is
already ready for re-use by other components (e.g. Contextual Search). Other
parts will need to be refactored first before we move them out of the Translate
component.



BUG=731845

Review-Url: https://codereview.chromium.org/2925733002
Cr-Commit-Position: refs/heads/master@{#482754}
Committed:
https://chromium.googlesource.com/chromium/src/+/97b0ab02964dac76c919c24cac8b...
==========

[commit-bot: I haz the power, 2017-06-27 21:42:23]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/97b0ab02964dac76c919c24cac8b...