Fix the TF tutorial to run against TF2.0 and TF1.x #4104

ic · 2019-10-11T08:45:16Z

TF 2.0 is now the stable release and breaks a few operations (different names, attributes, namespaces, etc). TF 1.x still evolves a little bit (1.15 coming), and the user community is likely to expect 1.x to still be supported.

This PR fixes the TF tutorial, as well as the TF frontend to Relay, so as to address #4101 . It is also the first part in supporting TF2.x completely.

Note for CR:

Individual commits contain errors and wrong contents, fixed in the end result. I hope we can squash the whole thing at merge time to hide my hesitation and early misunderstanding of the code base.
There is no new test. I do not see test coverage on the kind of change made here (basically in tests/python/frontend/tensorflow/test_forward.py around line 1103), so I guess it is more valuable for the community to get a fixed tutorial sooner. I am looking at how to test such change effectively, though, so may push something before the CR is completed (or require to complete).
For some reason the linter does not work for me. I am trying different container versions, but no avail on Darwin for now.

The following is the original contents for this PR, but way too broad. The target to support TF2.0 is tracked with issue #4102

This PR is WIP (read "not working yet"), as suggested by @srkreddy1238 to get started with TF 2 support

TF 2.0 is now the stable release and breaks a few operations (different names, attributes, namespaces, etc). TF 1.x still evolves a little bit (1.15 coming), and the user community is likely to expect 1.x to still be supported.

Here is a very early attempt at supporting TF 2.0, still compatible with TF 1.x, based on a first target: Get the TF tutorial back on track. There will be more to do.

Get the tutorial to run with TF 2.0.
[] Support operations at the intersection of TF 2.0 and TVM 0.6 (i.e. no new operation from TF yet).
[] Add test coverage against new APIs that have to be supported (e.g. resize has a new attribute, and no choice but to add it).

TF2.0 adds a `half_pixel_centers` attribute to the `resize` function in the image API. This commit completes the hooks in Relay's TF frontend. At the point of this commit, no new test yet. Also, this commit addresses solely the `resize` change. Other commits address other changes in TF2.0.

This looks cleaner than trying to detect the TF version.

This is a direct change, relying on the compat API provided by the TF team. This code will last as long as the compat API exists, so a "proper" support for TF1.x and 2.x will require more work in some future.

Explicit padding is a special case in TF2.0 (see reference linked below). Some models are serialized with that mode, and break TF support in TVM. Support is *partial* as EXPLICIT falls back to set padding on the Relay op, which only supports 2 values. At some point, padding may need to be extended to support 4 values, but that is out of scope of this support commit. Reference on EXPLICIT padding: tensorflow/tensorflow@ec81825#diff-1d1c0bb0a880f85b6164f71dbb2f446e

The `half_pixel_centers` attribute is a new feature in TF2.0. Earlier commits of mine mistakenly introduce them in the Relay API. This is probably not what Relay is expected to support, and the semantics of `half_pixel_centers` is unclear (to me, at least) at this point.

ic · 2019-10-22T01:37:14Z

@srkreddy1238 Not sure who to ask for reviews. Would you guide me on this one?

yzhliu · 2019-10-22T22:59:19Z

@yongwww @soiferj Could you also help to take a look?

jwfromm

Left one comment but otherwise LGTM.

python/tvm/relay/frontend/tensorflow.py

yongwww · 2019-10-22T23:56:08Z

python/tvm/relay/frontend/tensorflow.py

        return AttrCvt(op_name="resize",
-                       ignores=['Tdim'],
+                       ignores=['Tdim', 'half_pixel_centers'],


it would be nice if add half_pixel_centers in tvm resize op. If the result is different from tensorflow, we might need to raise error instead of ignoring

I think half_pixel_centers is the new name of align_corners (used in TF 1.X) that we do support in tvm resize.

then add transforms={'half_pixel_centers':'align_corners'} here should be working

It seems the two attributes serve different purposes. The doc says nothing really. The source code does half_pixel_centers processing is applied optionally on top of align_corners processing. E.g.

Eigen::Index in_y = std::min( (align_corners) ? static_cast<Eigen::Index>(roundf(scaler(y, height_scale))) : static_cast<Eigen::Index>(floorf(scaler(y, height_scale))), in_height - 1); if (half_pixel_centers) { in_y = std::max(static_cast<Eigen::Index>(0), in_y); }

Idea: How about a unit test that compares a TF model using that attribute to Relay ignoring the attribute? I was reading the test suite to add to this PR, when I saw little coverage on such kind of issue, and fixing the tutorial seemed good to release early.

I'll try a test today, but if you have some guidance/advice/preference, I'd be happy to hear it. It may just be a different PR.

tqchen · 2019-10-24T16:45:06Z

@yongwww please followup and https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

yongwww

left several comments, lgtm

python/tvm/relay/frontend/tensorflow.py

CR apache#4104 (comment) Addresses apache#4104

Complying without understanding the rationale for now.

An argument ignored for the wrong operation.

ic · 2019-10-31T10:36:57Z

Note: At present tests fail, but I cannot say why (they seem unrelated to changes here). The same errors happen on master. It may be my environment, but not fully sure yet. I'll trying, and if someone finds something that would be great.

yzhliu · 2019-11-08T16:33:13Z

@ic could you retrigger the CI by push a trivial commit?

ic · 2019-11-12T06:32:13Z

@yzhliu Sorry for my late reaction. Just syncing with master will do it!

yzhliu · 2019-11-12T23:52:36Z

Thanks @ic @yongwww @tqchen

* WIP Run the TF tutorial on TF2 * Remove debugger statement. * Complete the support for TF2.0's `resize`. TF2.0 adds a `half_pixel_centers` attribute to the `resize` function in the image API. This commit completes the hooks in Relay's TF frontend. At the point of this commit, no new test yet. Also, this commit addresses solely the `resize` change. Other commits address other changes in TF2.0. * Support TF2.0 in the tutorial by using the compat API. This looks cleaner than trying to detect the TF version. * Use the TF compat API, so as to support TF2.0. This is a direct change, relying on the compat API provided by the TF team. This code will last as long as the compat API exists, so a "proper" support for TF1.x and 2.x will require more work in some future. * Partial support for EXPLICIT padding introduced in TF2.0. Explicit padding is a special case in TF2.0 (see reference linked below). Some models are serialized with that mode, and break TF support in TVM. Support is *partial* as EXPLICIT falls back to set padding on the Relay op, which only supports 2 values. At some point, padding may need to be extended to support 4 values, but that is out of scope of this support commit. Reference on EXPLICIT padding: tensorflow/tensorflow@ec81825#diff-1d1c0bb0a880f85b6164f71dbb2f446e * Guard on checking for optional TF2.0 attribute. * Do not expect Relay to implement TF-specific attributes. The `half_pixel_centers` attribute is a new feature in TF2.0. Earlier commits of mine mistakenly introduce them in the Relay API. This is probably not what Relay is expected to support, and the semantics of `half_pixel_centers` is unclear (to me, at least) at this point. * Remove unclear comment. CR apache#4104 (comment) Addresses apache#4104 * Changes after review. Complying without understanding the rationale for now. * Fix the arguments set mistakenly. An argument ignored for the wrong operation.

ic added 2 commits October 11, 2019 15:31

WIP Run the TF tutorial on TF2

e63e80e

Remove debugger statement.

6ec2b28

ic changed the title ~~Support for TF 2.0, backward compatible with Tf 1.x~~ [WIP] Support for TF 2.0, backward compatible with Tf 1.x Oct 11, 2019

ic mentioned this pull request Oct 11, 2019

[Relay] Support for Tensorflow 2.0 #4102

Closed

ic added 7 commits October 11, 2019 22:22

Support TF2.0 in the tutorial by using the compat API.

eeb8650

This looks cleaner than trying to detect the TF version.

Use the TF compat API, so as to support TF2.0.

63ce005

This is a direct change, relying on the compat API provided by the TF team. This code will last as long as the compat API exists, so a "proper" support for TF1.x and 2.x will require more work in some future.

Merge branch 'master' into tf2

f00982f

Guard on checking for optional TF2.0 attribute.

3fa86a6

ic changed the title ~~[WIP] Support for TF 2.0, backward compatible with Tf 1.x~~ Fix the TF tutorial to run against TF2.0 and TF1.x Oct 18, 2019

ic mentioned this pull request Oct 22, 2019

Tensorflow tutorial broken with "unexpected keyword argument 'half_pixel_centers'" #4101

Closed

yzhliu added the status: need review label Oct 22, 2019

jwfromm approved these changes Oct 22, 2019

View reviewed changes

python/tvm/relay/frontend/tensorflow.py Outdated Show resolved Hide resolved

yongwww reviewed Oct 22, 2019

View reviewed changes

tqchen assigned yzhliu Oct 24, 2019

yongwww approved these changes Oct 24, 2019

View reviewed changes

python/tvm/relay/frontend/tensorflow.py Outdated Show resolved Hide resolved

ic added 5 commits October 25, 2019 10:25

Remove unclear comment.

5f63530

CR apache#4104 (comment) Addresses apache#4104

Merge branch 'master' of github.com:dmlc/tvm into tf2

8018983

Merge branch 'master' into tf2

92d5677

Changes after review.

f2fca5a

Complying without understanding the rationale for now.

Fix the arguments set mistakenly.

84702fe

An argument ignored for the wrong operation.

Merge branch 'master' of github.com:dmlc/tvm into tf2

92cfcb5

yzhliu approved these changes Nov 12, 2019

View reviewed changes

yzhliu merged commit e541c75 into apache:master Nov 12, 2019

tqchen mentioned this pull request Nov 16, 2019

[RELEASE][DRAFT] TVM v0.6 Release candidate #4259

Closed

This was referenced Dec 27, 2019

[Bugfix][Frontend][TF] Fix tutorial for TF<=1.12 compatible #4592

Closed

[Bugfix][Frontend][TF] Fix tutorial for TF<=1.12 compatible #4593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the TF tutorial to run against TF2.0 and TF1.x #4104

Fix the TF tutorial to run against TF2.0 and TF1.x #4104

ic commented Oct 11, 2019 •

edited

Loading

ic commented Oct 22, 2019

yzhliu commented Oct 22, 2019 •

edited

Loading

jwfromm left a comment

yongwww Oct 22, 2019

jwfromm Oct 23, 2019

yongwww Oct 24, 2019

ic Oct 25, 2019 •

edited

Loading

ic Oct 25, 2019

tqchen commented Oct 24, 2019

yongwww left a comment

ic commented Oct 31, 2019

yzhliu commented Nov 8, 2019

ic commented Nov 12, 2019

yzhliu commented Nov 12, 2019

Fix the TF tutorial to run against TF2.0 and TF1.x #4104

Fix the TF tutorial to run against TF2.0 and TF1.x #4104

Conversation

ic commented Oct 11, 2019 • edited Loading

ic commented Oct 22, 2019

yzhliu commented Oct 22, 2019 • edited Loading

jwfromm left a comment

Choose a reason for hiding this comment

yongwww Oct 22, 2019

Choose a reason for hiding this comment

jwfromm Oct 23, 2019

Choose a reason for hiding this comment

yongwww Oct 24, 2019

Choose a reason for hiding this comment

ic Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

ic Oct 25, 2019

Choose a reason for hiding this comment

tqchen commented Oct 24, 2019

yongwww left a comment

Choose a reason for hiding this comment

ic commented Oct 31, 2019

yzhliu commented Nov 8, 2019

ic commented Nov 12, 2019

yzhliu commented Nov 12, 2019

ic commented Oct 11, 2019 •

edited

Loading

yzhliu commented Oct 22, 2019 •

edited

Loading

ic Oct 25, 2019 •

edited

Loading