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

[Relay] Improve build error when no lowered funcs are produced #4132

Merged
merged 2 commits into from
Oct 17, 2019

Conversation

weberlo
Copy link
Contributor

@weberlo weberlo commented Oct 15, 2019

If you accidentally create a module with no lowered funcs via relay.build, interaction with the module down the road segfaults without an error message. For example, the annotated line in the program below would segfault, because I have accidentally passed params to relay.build, causing the entire program to be const-folded away.

x = relay.var('x', shape=(1, 16, 64, 64), dtype='float32')
conv_expr = relay.nn.conv2d(
        x, relay.var('w'),
        kernel_size=(3, 3),
        padding=(1, 1),
        channels=16)
func = relay.Function(relay.analysis.free_vars(conv_expr), conv_expr)
mod = relay.Module.from_expr(func)

x_data = np.random.uniform(size=(1, 16, 64, 64)).astype('float32')
w_data = np.random.uniform(size=(16, 16, 3, 3)).astype('float32')
params = { 'x': x_data, 'w': w_data }

with tvm.build_config(disable_vectorize=True):
    graph, c_mod, params = relay.build(mod, target="c", params=params)
print(c_mod.get_source())  # <-- crashes here

This PR adds an error message to prevent the generation of modules without lowered funcs.

CC @jroesch @zhiics @tqchen

target_host_,
BuildConfig::Current());
}
CHECK(lowered_funcs.size() != 0) << "no lowered funcs exist in the compiled module";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is a good point. But I am not sure if we really want to signal fatal here. We might just have an empty lowered_funcs in some circumstances. For example, we could just have a constant and invoke build. Since there is no primitive, we would not have the module, but it should still be valid.

Copy link
Contributor Author

@weberlo weberlo Oct 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I wasn't sure if there were cases where one would want to use a build result without a defined module. Maybe the right change would be to modify the ModuleNode* operator-> methods to check that the shared ptr isn't null?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think returning a nullptr is okay. Dereferencing a nullptr should crash. I would recommend logging a warning here so that users would know the module is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. A warning seems like a good compromise.

@tqchen
Copy link
Member

tqchen commented Oct 16, 2019

@weberlo please look into the ci error

@weberlo
Copy link
Contributor Author

weberlo commented Oct 16, 2019

I've switched the check in build_module.cc from fatal to a warning, as @zhiics suggested. @tqchen Do you think modifying the ModuleNode* operator-> methods to include a runtime nullptr check is a good idea?

I'm not sure what other code paths would generate null modules, but I think if we're generating them anywhere in our APIs, we should at least give users the tools to understand why things are crashing (i.e., by adding a helpful error message).

Copy link
Member

@zhiics zhiics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tmoreau89 tmoreau89 merged commit 3185e4a into apache:master Oct 17, 2019
anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Oct 17, 2019
…e#4132)

* Improve build error when no lowered funcs

* Switch from fatal to warning
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 18, 2019
…e#4132)

* Improve build error when no lowered funcs

* Switch from fatal to warning
petrex added a commit to petrex/tvm that referenced this pull request Oct 29, 2019
* master: (51 commits)
  [QNN][TFLite] Parsing QNN Add op. Adding MobilenetV2. (apache#4142)
  [CI] Pin NNPack pthreadtools version (apache#4152)
  Fix typo (apache#4144)
  [Relay][Frontend][TF] Add tensor array ops (apache#3798)
  [relay][vm] Separate VM runtime with executable (apache#4100)
  [PATCH] Fix undefined __floatdihf in libtvmruntime.so on aarch64. (apache#4119)
  [DOCKER] Pin torchvision==0.4.1 (apache#4140)
  [TOPI][x86] Cascade lake support. (apache#4123)
  [Relay] Improve build error when no lowered funcs are produced (apache#4132)
  [RUNTIME] Refactor object python FFI to new protocol. (apache#4128)
  Update PULL_REQUEST_TEMPLATE.md
  Adding support for dequantizing from int32 to float32. (apache#4130)
  [Relay][Training] Add and fix gradients (apache#4126)
  [QNN] Change default rouning to UPWARD. (apache#4131)
  Fix infer type of kernel in dense. (apache#4125)
  [Relay][AlterOpLayout] NHWC to NCHWc pad operator. (apache#4103)
  [ARITH] Fix lowering of floormod(x, y) != 0 (apache#4127)
  [RFC][RUNTIME] Introduce new object protocol. (apache#4115)
  [Relay][Topi] Disable conv NHWC pack int8. (apache#4038)
  Update task_cpp_unittest.sh
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants