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

Assertion to check if fake tensors have leaked into python #1447

Merged
merged 3 commits into from
Oct 6, 2022

Conversation

anijain2305
Copy link
Contributor

I am seeing a fake tensor in frame.f_locals which is confusing to me. And I think this is wrong. Fake tensor is internal, and when someone runs a graph with real inputs, none of the output should be a FakeTensor.

So, adding an assertion to catch it somewhat early to help debug and share some repros.

@jansel
Copy link
Contributor

jansel commented Oct 2, 2022

Seems fine, but tests are failing

Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

LGTM, but you have a couple of errors

@anijain2305
Copy link
Contributor Author

Yes, the goal was to run CI to uncover any such issues. Happy that CI uncovered atleast one. It seems to be Fake tensor issue as well. We should fix that one.

Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

I'm actually not that familiar with GraphArg or how it is used. Also, the test is failing correctly returns non-fake tensors which makes me a little suspect of this. We could also error in aot_eager instead.

Copy link
Contributor

@jansel jansel left a comment

Choose a reason for hiding this comment

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

Fix tests before landing

@anijain2305
Copy link
Contributor Author

anijain2305 commented Oct 6, 2022

@jansel Can you please review again as I added some __eq_ code?

Update - Removed the eq code because.I later realized that dataclasses have __eq__ implemented and it works for our purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants