[jira] [Comment Edited] (SOLR-11250) Add new LTR model which loads the model definition from the external resource

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[jira] [Comment Edited] (SOLR-11250) Add new LTR model which loads the model definition from the external resource

JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/SOLR-11250?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16164201#comment-16164201 ]

Yuki Yano edited comment on SOLR-11250 at 9/13/17 6:21 AM:
-----------------------------------------------------------

[~cpoerschke]
Thank you for reviewing my patch!.

bq. The abstract {{WrapperModel.fetchModelMap}} method taking a {{SolrResourceLoader}} argument but then there also being a concrete {{SolrResourceWrapperModel}} class somehow seemed odd to me.

This is remains of my trial-and-error...
At first, I planed to implement two classes, {{SolrResourceWrapperModel}} and {{BlobStoreWrapperModel}}. However, I found that using Blob API from LTR plugin was difficult and removed the latter class. Finally, {{SolrResourceWrapperModel}} was remained.
I agree with your change because the name of {{SolrResourceWrapperModel}} is now redundant and {{DefaultWrapperModel}} is  more clear.

bq. The patch also tentatively renames {{SolrResourceWrapperModel}} to {{DefaultWrapperModel}} and removes the format parameter in favour of a protected {{parseInputStream}} method called by {{fetchModelMap}}. In future a (say) YamlWrapperModel class could extend the DefaultWrapperModel and override its parseInputStream method. I've also factored out a protected {{openInputStream}} method called by {{fetchModelMap}} i.e. custom classes wishing to use something other than the SolrResourceLoader could do so by overriding that method. How does that sound?

I think your modification is better because new codes are simpler than previous ones and easier to understand.
On the other hand, we do not forget that "how to load resources" and "how to parse resources" are totally independent and separating these codes make it easy to extend in the future because we can reuse classes (This is why I implemented
 ModelParser as another class). However, maybe this separation is a bit much for SOLR-11250 (I think it is better to make new issue for implementing this if necessary).


was (Author: yuyano):
[~cpoerschke]
Thank you for reviewing my patch!.

bq. The abstract {{WrapperModel.fetchModelMap}} method taking a {{SolrResourceLoader}} argument but then there also being a concrete {{SolrResourceWrapperModel}} class somehow seemed odd to me.

This is remains of my trial-and-error...
At first, I planed to implement two classes, {{SolrResourceWrapperModel}} and {{BlobStoreWrapperModel}}. However, I found that using Blob API from LTR plugin was difficult and removed the latter class. Finally, {{SolrResourceWrapperModel}} was remained.
I agree with your change because the name of {{SolrResourceWrapperModel}} is now redundant and {{DefaultWrapperModel}} is  more clear.

bq. The patch also tentatively renames {{SolrResourceWrapperModel}} to {{DefaultWrapperModel}} and removes the format parameter in favour of a protected {{parseInputStream}} method called by {{fetchModelMap}}. In future a (say) YamlWrapperModel class could extend the DefaultWrapperModel and override its parseInputStream method. I've also factored out a protected {{openInputStream}} method called by {{fetchModelMap}} i.e. custom classes wishing to use something other than the SolrResourceLoader could do so by overriding that method. How does that sound?

I think your modification is better because new codes are simpler than previous ones and easy to understand.
On the other hand, we do not forget that "how to load resources" and "how to parse resources" are totally independent and separating these codes make it easy to extend in the future because we can reuse classes (This is why I implemented
 ModelParser as another class). However, maybe this separation is a bit much for SOLR-11250 (I think it is better to make new issue for implementing this if necessary).

> Add new LTR model which loads the model definition from the external resource
> -----------------------------------------------------------------------------
>
>                 Key: SOLR-11250
>                 URL: https://issues.apache.org/jira/browse/SOLR-11250
>             Project: Solr
>          Issue Type: Improvement
>      Security Level: Public(Default Security Level. Issues are Public)
>          Components: contrib - LTR
>            Reporter: Yuki Yano
>            Assignee: Christine Poerschke
>            Priority: Minor
>         Attachments: SOLR-11250_master.patch, SOLR-11250_master_v2.patch, SOLR-11250_master_v3.patch, SOLR-11250_master_v4.patch, SOLR-11250.patch, SOLR-11250.patch
>
>
> We add new model which contains only the location of the external model and loads it during the initialization.
> By this procedure, large models which are difficult to upload to ZooKeeper can be available.
> The new model works as the wrapper of existing models, and deligates APIs to them.
> We add two classes by this patch:
> * {{ExternalModel}} : a base class for models with external resources.
> * {{URIExternalModel}} : an implementation of {{ExternalModel}} which loads the external model from specified URI (ex. file:, http:, etc.).
> For example, if you have a model on the local disk "file:///var/models/myModel.json", the definition of {{URIExternalModel}} will be like the following.
> {code}
> {
>   "class" : "org.apache.solr.ltr.model.URIExternalModel",
>   "name" : "myURIExternalModel",
>   "features" : [],
>   "params" : {
>     "uri" : "file:///var/models/myModel.json"
>   }
> }
> {code}
> If you use LTR with {{model=myURIExternalModel}}, the model of {{myModel.json}} will be used for scoring documents.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]