-
Notifications
You must be signed in to change notification settings - Fork 654
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
Refactor ml extension support #1521
Conversation
This makes a few changes, specifically to fasttext, but it can apply to other DJL ml wrappers as well. First, it creates several passthrough utility classes with the goal that ml models can be loaded through the model zoo and run through the predictor. Next, it modifies the base of the ml construct to better support engines that have multiple applications. Each application now manifests as a type of SymbolBlock rather than a model. Then, the single model class can run any of them. The model is still created for each engine because it contains general loading functionality that can determine which block should be used to load the given target. It also has to update the fasttext JNI to support this. First, it fixes the modelType to actually return different results. Then, it modifies the predictProba method to use an ArrayList instead of an Array. The main difference is because it is possible to pass a topk of -1 in order to load all elements. However, this doesn't work with the previous array setup. Change-Id: I5fbaceb2ac4711a942b9d15dac6e6fed386dd46e
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.
Look good in general. But I have on doubt that if we should add fit
to FtClassificationBlock.
It has discoverability problem.
I think may add fit
to a utility class is better.
Also we might not want to expose .classify()
as public
@@ -125,7 +140,7 @@ public void testBlazingText() throws IOException, ModelException { | |||
model.load(modelFile); | |||
String text = | |||
"Convair was an american aircraft manufacturing company which later expanded into rockets and spacecraft ."; | |||
Classifications result = model.classify(text, 5); | |||
Classifications result = ((FtTextClassification) model.getBlock()).classify(text, 5); |
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 should be able to replace this test with Criteria as well.
I am not quite sure what you are thinking here. One of the problems I am also trying to deal with is what happens when the number of applications is quite large. For example, if we have an extension package wrapping sparkml, corenlp, smile, etc. I don't think it would fit well in a single utility for all of the training possibilities. Do you have a better solution?
I plan to leave it for the moment, because the predictor API is not quite as flexible. For example, I didn't try to figure out how to support predict while specifying topK, so that option is only available through classify. |
Codecov Report
@@ Coverage Diff @@
## master #1521 +/- ##
============================================
- Coverage 72.08% 70.68% -1.41%
- Complexity 5126 5349 +223
============================================
Files 473 500 +27
Lines 21970 23369 +1399
Branches 2351 2552 +201
============================================
+ Hits 15838 16518 +680
- Misses 4925 5569 +644
- Partials 1207 1282 +75
Continue to review full report at Codecov.
|
Change-Id: Iadf598d930dca954ee1a0f450012cd4b4b2baab5
This makes a few changes, specifically to fasttext, but it can apply to other
DJL ml wrappers as well. First, it creates several passthrough utility classes
with the goal that ml models can be loaded through the model zoo and run through
the predictor.
Next, it modifies the base of the ml construct to better support engines that
have multiple applications. Each application now manifests as a type of
SymbolBlock rather than a model. Then, the single model class can run any of
them. The model is still created for each engine because it contains general
loading functionality that can determine which block should be used to load the
given target.
It also has to update the fasttext JNI to support this. First, it fixes the
modelType to actually return different results. Then, it modifies the
predictProba method to use an ArrayList instead of an Array. The main difference
is because it is possible to pass a topk of -1 in order to load all elements.
However, this doesn't work with the previous array setup.
Change-Id: I5fbaceb2ac4711a942b9d15dac6e6fed386dd46e