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

IGNITE-10234: Inference workflow for ML #5415

Closed
wants to merge 10 commits into from

Conversation

@dmitrievanthony
Copy link
Contributor

dmitrievanthony commented Nov 16, 2018

No description provided.

@dmitrievanthony dmitrievanthony force-pushed the gridgain:ignite-10234 branch from 8ec74eb to 89ac2fe Nov 19, 2018
Copy link
Contributor

avplatonov left a comment

Thanks for your contribution! Please see my comments.

/**
* Signature that defines input/output types in Protobuf.
*/
public class ModelSignature implements Serializable {

This comment has been minimized.

Copy link
@avplatonov

avplatonov Nov 20, 2018

Contributor

i think that ModelSignature and ModelStorage classes formally don't relate to this PR. Such aspect doesn't provoke me, but maybe other will be against such decision and they will require to create other ticket for this task.

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 20, 2018

Author Contributor

The key point here is to define a skeleton for inference/serve that handles TensorFlow case. I think that it makes sense to keep this part in this PR so that the whole inference infrastructure is defined without contradictions.

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

Great to separate on two PRs, agree with @avplatonov

* (see {@link InfModelParser}) to build a model.
*/
@FunctionalInterface
public interface AsyncInfModelBuilder {

This comment has been minimized.

Copy link
@avplatonov

avplatonov Nov 20, 2018

Contributor

Maybe you merge AsyncInfModelBuilder and SyncModelBuilder into one hierarchy?
Like

interface ModelBuilder {
... InfModel<I, O> build..
}

and

AsyncModelBuilder extends ModelBuilder<Future>

just for reading comfort and maybe someone will want to abstract from AsyncModelBuilder to ModelBuilder

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 20, 2018

Author Contributor

Doesn't look like it works. The type is defined on method-level and it's not defined as a type of a class.

* @param <I> Type of model input.
* @param <O> Type of model output.
*/
private static class IgniteDistributedInfModelService<I extends Serializable, O extends Serializable>

This comment has been minimized.

Copy link
@avplatonov

avplatonov Nov 20, 2018

Contributor

Maybe you should place code lower than DistributedInfModel
just for reading comfort - firstly you use DistributedInfModel and write its definition, and secondly, using Service in InfModel write Service code after

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 20, 2018

Author Contributor

Done.

@Override public void init(ServiceContext ctx) {
Ignite ignite = Ignition.localIgnite();

reqQueue = ignite.queue(String.format(INFERENCE_REQ_QUEUE_NAME_PATTERN, suffix), QUEUE_CAPACITY, queueCfg);

This comment has been minimized.

Copy link
@avplatonov

avplatonov Nov 20, 2018

Contributor

Let's use more long names INFERENCE_REQUEST_QUEUE_NAME_PATTERN

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 20, 2018

Author Contributor

Done.

while (!ctx.isCancelled()) {
I req;
try {
req = reqQueue.take();

This comment has been minimized.

Copy link
@avplatonov

avplatonov Nov 20, 2018

Contributor

Premature optimization:
If "mdl.predict(req);" will be cheap call then we can be slow downed due to parks/unparks on "reqQueue.take()"

Maybe we should create some benchmark for this code

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 20, 2018

Author Contributor

I would prefer do not make any optimizations before fundamental queue problems are not solved (see IGNITE-10250).

ignite.services().deployMultiple(
String.format(INFERENCE_SERVICE_NAME_PATTERN, suffix),
new IgniteDistributedInfModelService<>(reader, parser, suffix),
instances,

This comment has been minimized.

Copy link
@avplatonov

avplatonov Nov 20, 2018

Contributor

are you sure that totalCnt == maxPerNodeCnt ?

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 20, 2018

Author Contributor

Good point. I moved this parameter on API level, please have a look.

API, rearranged code in IgniteDistributedInfModelBuilder.
examples (local, threaded and distributed), add example of inference
for linear regression model.
System.out.println(">>> Linear regression model: " + mdl);

System.out.println(">>> Preparing model reader and model parser.");
InfModelReader reader = new InMemoryInfModelReader(mdl);

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

Why not simplify to ModelReader

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

We have kind of another Model interface in our code base. From my perspective it would be better to have prefix Inf to distinguish them.


System.out.println(">>> Preparing model reader and model parser.");
InfModelReader reader = new InMemoryInfModelReader(mdl);
InfModelParser<Vector, Double> parser = new IgniteFunctionInfModelParser<>();

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

Why not to simplify to Model Parser

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

See comment above.


InfModelReader reader = new DirectoryInfModelReader(mdlRsrc.getPath());

InfModelParser<double[], Long> parser = new TensorFlowSavedModelInfModelParser<double[], Long>("serve")

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

TFModelParser is better

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

See comment above.

InfModelParser<double[], Long> parser = new TensorFlowSavedModelInfModelParser<double[], Long>("serve")

.withInput("Placeholder", doubles -> {
float[][][] reshaped = new float[1][28][28];

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

is it a Tensor-like structure here?

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

Yep.

InfModelReader reader = new DirectoryInfModelReader(mdlRsrc.getPath());

InfModelParser<double[], Long> parser = new TensorFlowSavedModelInfModelParser<double[], Long>("serve")

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

delete this gap

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

Done.

/**
* Model reader that reads directory and serializes it using {@link DirectorySerializer}.
*/
public class DirectoryInfModelReader implements InfModelReader {

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

Seems like we could avoid separate classm and check path.isDirectory in common approach

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

Good point, done (see FileSystemInfModelReader).

*/
public <T extends Serializable> InMemoryInfModelReader(T obj) {
try (ByteArrayOutputStream baos = new ByteArrayOutputStream();
ObjectOutputStream oos = new ObjectOutputStream(baos)) {

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

format code

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

It's formatted, isn't it?

/**
* Model descriptor storage based on local hash map.
*/
public class HashMapModelDescriptorStorage implements ModelDescriptorStorage {

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

Wow, very strange to name something based on Java Implementation, please rename LocalModelDescriptorStorage

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

Done.

/**
* Model descriptor storage based on Apache Ignite cache.
*/
public class IgniteModelDescriptorStorage implements ModelDescriptorStorage {

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

Rename to DistributedModelDescriptorStorage

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

We might have distributed storages based on other underlying platforms, you know...

*/
@RunWith(Suite.class)
@Suite.SuiteClasses({
IgniteDistributedInfModelBuilderTest.class,

This comment has been minimized.

Copy link
@zaleslaw

zaleslaw Nov 21, 2018

Contributor

Seems like not full test coverage (but I'm not sure)

This comment has been minimized.

Copy link
@dmitrievanthony

dmitrievanthony Nov 21, 2018

Author Contributor

There are no tests for DTO objects and dummy code like wrappers above HashMap and IgniteCache.

into Single FileSystemReader, HashMapModelDescriptorStorage renamed to
LocalModelDescriptorStorage, some refactoring is done.
@asfgit asfgit closed this in ec11864 Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.