Skip to content

[AutoDiff] Add @differentiable attribute SILGen assertion. #27650

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 13, 2019
Merged

[AutoDiff] Add @differentiable attribute SILGen assertion. #27650

merged 1 commit into from
Oct 13, 2019

Conversation

dan-zheng
Copy link
Contributor

Previously, @differentiable attribute SILGen lowering was skipped if the
attribute did not have resolved parameter indices. This led to unexpected
and difficult-to-debug bugs like TF-888.

Re-add the assertion for robustness; I suspect this may be possible after
#27613.

@dan-zheng
[AutoDiff] Add @differentiable attribute SILGen assertion.
264b50e 
Previously, `@differentiable` attribute SILGen lowering was skipped if the
attribute did not have resolved parameter indices. This led to unexpected
and difficult-to-debug bugs like TF-888.

Re-add the assertion for robustness.
@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Oct 13, 2019
@dan-zheng
Copy link
Contributor Author

dan-zheng commented Oct 13, 2019 •
edited
Loading

Currently, we have a lack of cross-module differentiation tests. (One reason is it's difficult to predict which reproducers may cause cross-module bugs.)

I'll run the extended test suite (tensorflow/swift-apis, tensorflow/swift-models, fastai/swiftai, etc) to verify whether it's okay to re-enable the assertion.

@dan-zheng dan-zheng requested a review from rxwei October 13, 2019 06:45
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

rxwei
rxwei reviewed Oct 13, 2019
View reviewed changes
Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

I think this issue was specific to deserialization and it made tensorflow/swift-apis fail to compile. @eaplatanios added this continue to make things work. I don't yet know what the right solution is without knowing the underlying problem, and I suspect it will fail tensorflow/swift-apis compilation again. Let's wait on your test results.

@dan-zheng
Copy link
Contributor Author

I think this issue was specific to deserialization and it made tensorflow/swift-apis fail to compile. @eaplatanios added this continue to make things work. I don't yet know what the right solution is without knowing the underlying problem, and I suspect it will fail tensorflow/swift-apis compilation again. Let's wait on your test results.

That matches my understanding. I tracked down the original deserialization issue.

I found that the continue caused TF-888: some @differentiable attributes from non-primary files did not get SILGen'd to [differentiable] attributes. Based on local testing, I believe the fix #27613 may allow this assertion to be re-enabled.

rxwei
rxwei approved these changes Oct 13, 2019
View reviewed changes
Copy link
Contributor

@rxwei rxwei 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 for the explanation!

@dan-zheng
Copy link
Contributor Author

Extended CI has passed. Merging.

@dan-zheng dan-zheng merged commit 9c76a29 into swiftlang:tensorflow Oct 13, 2019
@dan-zheng dan-zheng deleted the diff-attr-assert-resolved-param-indices branch October 13, 2019 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rxwei rxwei rxwei approved these changes

Assignees
No one assigned
Labels
tensorflow This is for "tensorflow" branch PRs.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dan-zheng @rxwei