[Relay] Prepare for new plan_devices.cc (part II) #9130

mbs-octoml · 2021-09-26T15:14:25Z

These changes came from changing #9038 to use
tvm.parser.fromtext instead of manual AST construction.

Demote FunctionOnDeviceAttrs to just a pair of DictAttrs entries so
that the parser will understand them on Function definitions.
Connect some special operators to their attributes so parsing understands them
at call sites.
Don't silently ignore attributes during parsing.
Implement OptFunctionOnDevice so won't add device annotations for kUnknownDeviceType.
Allow the parser to be given an initial metadata map to support examples which
need constants.
More DLOG -> VLOG conversions to reduce debug clutter.

These changes came from changing apache#9038 to use tvm.parser.fromtext instead of manual AST construction. - Demote FunctionOnDeviceAttrs to just a pair of DictAttrs entries so that the parser will understand them on Function definitions. - Connect some special operators to their attributes so parsing understands them at call sites. - Don't silently ignore attributes during parsing. - Implement OptFunctionOnDevice so won't add device annotations for kUnknownDeviceType. - Allow the parser to be given an initial metadata map to support examples which need constants. - More DLOG -> VLOG conversions to reduce debug clutter.

Mousius

Overall looks straight forward, just missing a test case and minor tweaking but otherwise seems non-controversial 😸

Mousius · 2021-09-26T19:52:28Z

python/tvm/parser/__init__.py

-def parse(source, source_name="from_string"):
-    return _ffi_api.ParseModule(source_name, source)
+def parse(source, source_name="from_string", init_module=None, init_meta_table=None):
+    if init_meta_table is None:


This can be defaulted in the arguments.

Done. (I had the obvious default but clion was warning about mutable defaults and suggest this instead.)

Oh, the linter complains about it too "Dangerous default value {} as argument" - I'll go back to the silly form.

You can't default Python arguments to anything but atomic values. If you use an object or other aggregate data structure the default will be allocated a single time, and only a single time. If you happen to mutate it you will observe the entire history of mutations across all invocations of the function inside the process.

Ah, thanks! Not so silly a rule after all.

I'd suggest the language behaviour is the silly part? 👍

I'd suggest the language behaviour is the silly part? 👍

Yeah its not a good design, but alas you gotta love the Python you are with 😆 it has burned many people many times, super sharp edge.

Mousius · 2021-09-26T19:55:36Z

src/relay/op/annotation/annotation.cc

@@ -92,6 +91,7 @@ RELAY_REGISTER_OP("on_device")
    .add_argument("data", "Tensor", "The input data.")
    .set_support_level(10)
    .add_type_rel("Identity", IdentityRel)
+    .set_attrs_type_key("relay.attrs.OnDeviceAttrs")


Is this still required now you've split the attributes?

Yes, this is for the "on_device" operator which still uses OnDeviceAttrs on every CallNode. The split only applies to Function DictAttrs attrs which apply on definitions.

Ahhh, how very exciting 👍

Mousius · 2021-09-26T20:08:32Z

src/parser/parser.cc

+  for (const auto& pair : init_meta_table) {
+    Array<ObjectRef> items;
+    if (meta_data_table.count(pair.first)) {
+      items = meta_data_table[pair.first];
+    }
+    for (const auto& obj : pair.second) {
+      items.push_back(obj);
+    }
+    meta_data_table.Set(pair.first, items);
+  }


I can't see any test cases for this logic, is it possible for you to add one so I can see it in action? 😸

Good call - done.

Mousius · 2021-09-26T20:10:37Z

include/tvm/relay/attrs/function.h

- * See also OnDeviceAttrs in include/tvm/relay/attrs/annotation.h for the companion "on_device"
- * call attributes.
- */
-struct FunctionOnDeviceAttrs : public tvm::AttrsNode<FunctionOnDeviceAttrs> {


It's a shame about this, this increases my nervousness around apache/tvm-rfcs#29 having issues as well 😿

On principle it would be nice if IRModule can be round tripped via the text form (both starting from text and starting from IRModule), so we'll need to push the attrs field addition into the parser/pretty printer. Ideally that would just be a matter of calling ParseAttrs as we already do for function calls, but yeah there's no support for anything other than primitive literals in that.

I'll admit that pushes me more towards introducing a CompilerConfig class to collect targets etc and passing it explicitly rather than embedding it in the IRModule. Maybe take that back to #29?

Mhm, not a discussion for here, and I'm fairly sure we can adopt it later if necessary - even if it's not carved into the stone tablet of the RFC.

jroesch

I already reviewed in the other PR.

Mousius

LGTM! 👍

* main: Fix flaky NMS test by making sure scores are unique (apache#9140) [Relay] Merge analysis/context_analysis.cc and transforms/device_annotation.cc (apache#9038) [LLVM] Make changes needed for opaque pointers (apache#9138) Arm(R) Ethos(TM)-U NPU codegen integration (apache#8849) [CI] Split Integration tests out of first phase of pipeline (apache#9128) [Meta Schedule][M3b] Runner (apache#9111) Fix Google Mock differences between Ubuntu 18.04 and 16.04 (apache#9141) [TIR] add loop partition hint pragma (apache#9121) fix things (apache#9146) [Meta Schedule][M3a] SearchStrategy (apache#9132) [Frontend][PyTorch] support for quantized conv_transpose2d op (apache#9133) [UnitTest] Parametrized test_conv2d_int8_intrinsics (apache#9143) [OpenCL] Remove redundant visit statement in CodeGen. (apache#9144) [BYOC] support arbitrary input dims for add/mul/relu of dnnl c_src codegen (apache#9127) [Relay][ConvertLayout] Support for qnn.conv2d_transpose (apache#9139) add nn.global_avgpool to fq2i (apache#9137) [UnitTests] Enable minimum testing on Vulkan target in CI (apache#9093) [Torch] Support returning quantized weights and bias for BYOC use cases (apache#9135) [Relay] Prepare for new plan_devices.cc (part II) (apache#9130) [microTVM][Zephyr] Add MIMXRT1050 board support (apache#9068)

* Prepare for new plan_devices.cc (part II) These changes came from changing apache#9038 to use tvm.parser.fromtext instead of manual AST construction. - Demote FunctionOnDeviceAttrs to just a pair of DictAttrs entries so that the parser will understand them on Function definitions. - Connect some special operators to their attributes so parsing understands them at call sites. - Don't silently ignore attributes during parsing. - Implement OptFunctionOnDevice so won't add device annotations for kUnknownDeviceType. - Allow the parser to be given an initial metadata map to support examples which need constants. - More DLOG -> VLOG conversions to reduce debug clutter. * [checkpoint] Keep existing ParseModule ffi to simplify rust bindings * [checkpoint] Address Christopher's comments. * [checkpoint] Andrew's comments from apache#9038 * [checkpoint] Jared's comments from apache#9038 * [checkpoint] Woops, forgot rename.

mbs-octoml requested review from anijain2305, areusch, comaniac, jroesch, junrushao, jwfromm, MarisaKirisame, mbrookhart, merrymercy, slyubomirsky, tqchen, vinx13, wweic, yzhliu, zhiics and ZihengJiang as code owners September 26, 2021 15:14

mbs-octoml mentioned this pull request Sep 26, 2021

[Relay] Merge analysis/context_analysis.cc and transforms/device_annotation.cc #9038

Merged

Mousius requested changes Sep 26, 2021

View reviewed changes

mbs-octoml added 2 commits September 27, 2021 10:20

[checkpoint] Keep existing ParseModule ffi to simplify rust bindings

3cbeb81

[checkpoint] Address Christopher's comments.

4d7c798

jroesch approved these changes Sep 27, 2021

View reviewed changes

mbs-octoml added 3 commits September 27, 2021 15:02

[checkpoint] Andrew's comments from apache#9038

5553be2

[checkpoint] Jared's comments from apache#9038

738bb29

[checkpoint] Woops, forgot rename.

72b0784

Mousius approved these changes Sep 28, 2021

View reviewed changes

jroesch merged commit 163322c into apache:main Sep 28, 2021

mbs-octoml deleted the mbs-pre-plan-devices branch September 28, 2021 18:30

junrushao mentioned this pull request Nov 2, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay] Prepare for new plan_devices.cc (part II) #9130

[Relay] Prepare for new plan_devices.cc (part II) #9130

mbs-octoml commented Sep 26, 2021

Mousius left a comment

Mousius Sep 26, 2021

mbs-octoml Sep 27, 2021

mbs-octoml Sep 27, 2021

jroesch Sep 27, 2021

mbs-octoml Sep 27, 2021

Mousius Sep 28, 2021

jroesch Sep 28, 2021

Mousius Sep 26, 2021

mbs-octoml Sep 27, 2021

Mousius Sep 28, 2021

Mousius Sep 26, 2021

mbs-octoml Sep 27, 2021

Mousius Sep 26, 2021

mbs-octoml Sep 27, 2021

Mousius Sep 28, 2021

jroesch left a comment

Mousius left a comment

[Relay] Prepare for new plan_devices.cc (part II) #9130

[Relay] Prepare for new plan_devices.cc (part II) #9130

Conversation

mbs-octoml commented Sep 26, 2021

Mousius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroesch left a comment

Choose a reason for hiding this comment

Mousius left a comment

Choose a reason for hiding this comment