KonduitAI / konduit-serving

Enterprise runtime for machine learning models
https://serving.konduit.ai/
Apache License 2.0
47 stars 15 forks source link

Reduce redundancy in JSON/YAML step config for model type #213

Open AlexDBlack opened 4 years ago

AlexDBlack commented 4 years ago

Consider the YAML here: https://serving.oss.konduit.ai/examples/python/tensorflow-model-serving/tf-mnist

steps:
  tensorflow_step:
    type: TENSORFLOW

This is redundant - why do I need to specify the type if I've already specified tensorflow_step?

Same thing here: https://serving.oss.konduit.ai/examples/python/dl4j

steps:
  dl4j_mln_step:
    type: MULTI_LAYER_NETWORK
ShamsUlAzeem commented 4 years ago

Hmm, one way to solve this is to have a list of objects instead of a dictionary for steps.

AlexDBlack commented 4 years ago

@ShamsUlAzeem Looking at your PR, I'm thinking maybe there was a misunderstanding on my part as to the format

I understood the format to be:

steps:
  <step_type_name>:
    type: <step_type_enum>
    ...

where actually corresponded to exactly one thing for DL4J, exactly one thing for TF, etc. Hence specifying both <step_type_name> and <step_type_enum> being redundant as they would always have to align.

Whereas now, looking at ServingConfig.json again (which has List and hence no way to actually load/store the <step_type_name> part), I understand the <step_type_name> part to be an arbitrary user-defined name, that has no specific meaning beyond being a label for the user? Is that correct?

ShamsUlAzeem commented 4 years ago

@AlexDBlack Yeah that was only supposed to be a label for the step. We weren't really storing it anywhere internally. The actual data that was important was the type enum.

Is it important that we allow the user to put labels for the steps inside the config even though it's not used internally? Also, I wasn't sure if the steps dictionary was read in order while being read from the yaml or json config file so, by doing that the steps order could change without us knowing. So, having a list might be a better option here.

For labelling the steps the users might wanna add comments of their own.

ShamsUlAzeem commented 4 years ago

Btw we can use the previous dictionary format as well here.

AlexDBlack commented 4 years ago

OK, so it's a user label only (not used).

I kind of do like the idea of having user defined labels (that we actually use), for a few reasons:

But even so, that doesn't need to be as a wrapper object.

Thinking about this, one option I like here is to use a "wrapper object" style (like what I thought it originally was):

steps:
  ModelStep:
    step_name: "my model step"
    model_loading_path: ../data/myModel.pb
    ....
  PythonStep:
    step_name: "my postprocessing step"
    ...

i.e., where "ModelStep" and "PythonStep" are one of a set of predefined names that identify the type - {ModelStep, PmmlStep, PythonStep, TransformProcessStep, ...}. We should be able to do that with Java/Jackson using "subtypes encoded as wrapper object" functionality.

I think that would be clearer for users while allowing a fairly straightforward extension to graph style pipelines later.

I'm also thinking we should flatten the configuration heirarchy a bit to make it easier for users to use. At present, at least in Java, we have ModelStep, which has a ModelConfig, which in turn has a ModelConfigType. That's 3 layers deep. Why not flatten all of that into a

i.e., instead of this:

.step(ModelStep.builder().modelConfig(DL4JConfig.builder().modelConfigType(ModelConfigType.computationGraph("/my/model/path.bin")).build()).build())

do this:

.step(DL4JStep.builder().path("/my/model/path.bin").build())


I'm also kind of confused about the difference between the YAML I see in the docs and the YAML I see generated from Java. For example this:

InferenceConfiguration conf = InferenceConfiguration.builder()
 .step(ModelStep.builder().modelConfig(DL4JConfig.builder().modelConfigType(ModelConfigType.computationGraph("/my/model/path.bin")).build()).build())
.servingConfig(ServingConfig.builder().httpPort(12345).logTimings(true).build())
.build();

Produces this:

---
servingConfig:
  ... [ommitted for brevity]
steps:
- !<ModelStep>
  inputColumnNames: {}
  inputNames: []
  inputSchemas: {}
  modelConfig: !<DL4JConfig>
    modelConfigType:
      modelLoadingPath: "/my/model/path.bin"
      modelType: "COMPUTATION_GRAPH"
  outputColumnNames: {}
  outputNames: []
  outputSchemas: {}
  parallelInferenceConfig:
    batchLimit: 32
    inferenceMode: "BATCHED"
    maxTrainEpochs: 1
    queueLimit: 64
    workers: 1

Which is very different to this: https://serving.oss.konduit.ai/examples/python/dl4j (under "Configure the step")

steps:
  dl4j_mln_step:
    type: MULTI_LAYER_NETWORK
    model_loading_path: ../data/multilayernetwork/SimpleCNN.zip
    input_names: 
    - image_array
    output_names: 
    - output
    input_data_types:
      image_array: FLOAT
    parallel_inference_config: 
      workers: 1

@ShamsUlAzeem are the Java and Python YAML representations even compatible at this point? I think that's something need tests for - Java -> Python -> Java -> Python - and ensure the same at each point