[tvm4j] add GraphRuntime #1472
[tvm4j] add GraphRuntime #1472
Conversation
|
please request reviews |
| */ | ||
| public static GraphModule create(String graphJson, Module libmod, TVMContext ctx) { | ||
| Module graphModule = null; | ||
| if (ctx.deviceType >= RPC.RPC_SESS_MASK) { |
There was a problem hiding this comment.
Maybe combine these into if P and Q instead of two separate if statements?
There was a problem hiding this comment.
I feel the current statement is more readable, the ifs inside are more about assert, rather than control flows.
| value.copyTo(input); | ||
| } | ||
| fsetInput.pushArg(key).pushArg(input).invoke(); | ||
| return this; |
There was a problem hiding this comment.
Is there a particular use for returning this?
There was a problem hiding this comment.
a common design in Java world, which enable users to do something like g.setInputs(..).setInputs(...).run()
| from tvm.contrib import graph_runtime | ||
|
|
||
| def dump_graph_lib(target_dir): | ||
| n = 4 |
There was a problem hiding this comment.
I get that this is math inside of a test script but perhaps we want to avoid using single-letter variable names? We could change to dim or something like that.
| n = 4 | ||
| A = tvm.placeholder((n,), name='A') | ||
| B = tvm.compute(A.shape, lambda *i: A(*i) + 1.0, name='B') | ||
| s = tvm.create_schedule(B.op) |
| @@ -57,8 +57,7 @@ public NDArrayBase copyTo(NDArrayBase target) { | |||
| /** | |||
| * Release the NDArray memory. | |||
| * <p> | |||
| * We highly recommend you to do this manually since the GC strategy is lazy | |||
| * and `finalize()` is not guaranteed to be called when GC happens. | |||
There was a problem hiding this comment.
Is there a particular reason why this has been deleted? This info might be useful to keep.
There was a problem hiding this comment.
actually finalize() is guaranteed to be called finally. it just cannot be called immediately.
| */ | ||
|
|
||
| package ml.dmlc.tvm; | ||
|
|
There was a problem hiding this comment.
Perhaps include a brief explanation as you did with other classes.
| * @param value The input value. | ||
| * @return self. | ||
| */ | ||
| public GraphModule setInput(int key, NDArray value) { |
There was a problem hiding this comment.
Do we want to support batch set inputs?
There was a problem hiding this comment.
I think batched inputs can still be described as NDArray.
There was a problem hiding this comment.
But here we only supports a single input key. We may want a map for multiple inputs.
There was a problem hiding this comment.
you can do g.setInput(k1, v1).setInput(k2, v2).setInput...
There was a problem hiding this comment.
Yes. User can always set input one by one. I'm thinking whether we can make the API simpler. Maybe we can do this later if it requires significant extra work.
There was a problem hiding this comment.
I prefer not to provide another two functions� setInput(Map<String, NDArray>) and setInput(Map<Integer, NDArray>) since now users need to construct Map using Map.put(k, v) first - which is actually less convenient.
| * @param value The input value | ||
| * @return self. | ||
| */ | ||
| public GraphModule setInput(String key, NDArray value) { |
There was a problem hiding this comment.
Combine these two setInputs using a generic method? It seems the bodies are the same.
There was a problem hiding this comment.
how... the types are different.
There was a problem hiding this comment.
Never mind. I was thinking sth like, public <T> GraphModule setInput(T key, NDArray value). Not quite familiar with JAVA...
There was a problem hiding this comment.
in java, if we do in this way we have to check and cast T to int/String manually before passing to pushArg. Moreover, users will get confused with what type is exactly supported for key since it is not specified in the function signature.
|
@markrogersjr @kevinthesun @zhiics Thanks for reviewing. Once you feel it is good to merge, please explicitly approve it: https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly |
|
Thanks @yzhliu @zhiics @kevinthesun @markrogersjr this is now merged. |
GraphRuntime for Java.