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][Frontend][Keras] NHWC import support. #4899

Merged
merged 5 commits into from
Feb 18, 2020
Merged

Conversation

jwfromm
Copy link
Contributor

@jwfromm jwfromm commented Feb 17, 2020

Currently the Keras importer can only output models with NCHW layout. This PR adds a layout argument to the Keras importer that allows NHWC to be used instead. This is useful for platforms that may have better NHWC than NCHW performance such as the Raspberry Pi.

A few notes:

  • Xception is not being tested in NHWC mode as I found that it was causing a strange cuda error when targeting GPU. On CPU, it works fine however so I suspect this is an issue with the nhwc separable convolutions in Topi rather than the importer.
  • I made a few changes to the test script to allow eager execution (and TF2.0) to work.

@jwfromm jwfromm requested a review from masahi February 17, 2020 07:26
@jwfromm
Copy link
Contributor Author

jwfromm commented Feb 17, 2020

@masahi, @soiferj Can you take a look at this PR?

@masahi
Copy link
Member

masahi commented Feb 17, 2020

I'm not familiar with keras, but I wonder why keras use NCHW while tensorflow uses NHWC? (I'm not familiar with tf either). I heard that since TF 2.0, keras became their official frontend or something like that.

@masahi masahi self-assigned this Feb 17, 2020
python/tvm/relay/frontend/keras.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/keras.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/keras.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/keras.py Show resolved Hide resolved
@jwfromm
Copy link
Contributor Author

jwfromm commented Feb 17, 2020

@masahi, Keras does use NHWC by default just like TensorFlow. However, because TVM is better optimized for NCHW, the Keras frontend previously converted all layers from NHWC to NCHW. This option allows us to keep the layers in their original format.

@jwfromm
Copy link
Contributor Author

jwfromm commented Feb 17, 2020

@comaniac I think I've fixed all the style issues you pointed out.

Copy link
Contributor

@comaniac comaniac left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks :)

Copy link
Contributor

@anijain2305 anijain2305 left a comment

Choose a reason for hiding this comment

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

I would suggest to use ConvertLayout. ConvertLayout was designed for exactly this purpose.

@jwfromm
Copy link
Contributor Author

jwfromm commented Feb 17, 2020

@anijain2305, I think you're right that for most cases ConvertLayout will achieve something similar. However, there are some benefits to having layers directly parsed as NHWC. I actually did this PR because I'm working on a project that uses some funky custom Keras layers and it's very useful to parse them directly in NHWC.

@tqchen
Copy link
Member

tqchen commented Feb 17, 2020

Given that the model is in native NHWC, perhaps it is better to do it in another way around(note that the old approach converts to NCHW on the fly), ingest as NHWC as in this PR and then call ConvertLayout

@jwfromm
Copy link
Contributor Author

jwfromm commented Feb 17, 2020

@tqchen, you're right that would be a more direct way to apply ConvertLayout. If everyone else would prefer it, I could change the Keras frontend to only output in NHWC and then we would rely on ConvertLayout to get models into NCHW.

@anijain2305
Copy link
Contributor

I see. Yes, I meant that we should retain native framework format (NHWC in this case). And use ConvertLayout to change the layout to desired layout (passed on by relay.from_keras API).

@jwfromm Yes, in my opinion, that is the cleanest way forward. Same thing needs to happen for TF framework. I can take care of that. Will appreciate if you can take care of Keras.

@tqchen
Copy link
Member

tqchen commented Feb 18, 2020

That sounds good.

@tqchen tqchen merged commit 9d64654 into apache:master Feb 18, 2020
@tqchen
Copy link
Member

tqchen commented Feb 18, 2020

Merging this for now as this PR is self contained. It would be great if we can have a followup PR that updates the frontend to use ConvertLayout from NHWC -> NCHW so we won't have to support two path in the frontend

Thanks @anijain2305 @jwfromm @comaniac

@jwfromm jwfromm deleted the keras_nhwc branch February 18, 2020 19:44
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 26, 2020
* Basic test working

* Almost all tests working.

* all tests passing.

* Fixed lint.

* Improved Style.
alexwong pushed a commit to alexwong/tvm that referenced this pull request Feb 28, 2020
* Basic test working

* Almost all tests working.

* all tests passing.

* Fixed lint.

* Improved Style.
zhiics pushed a commit to neo-ai/tvm that referenced this pull request Mar 2, 2020
* Basic test working

* Almost all tests working.

* all tests passing.

* Fixed lint.

* Improved Style.
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.

5 participants