Skip to content

Conversation

Chillee
Copy link
Collaborator

@Chillee Chillee commented Oct 28, 2020

Probably works :)

Fixes #46872

Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

This is actually pretty close to what I was proposing, just need to get the specifics ironed out. Can you:

  1. Add the test case from the issue to test_fx.py
  2. Work out the issues with this on the other tests

@dr-ci
Copy link

dr-ci bot commented Oct 29, 2020

💊 CI failures summary and remediations

As of commit 6cc39cb (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 29 times.

@Chillee Chillee changed the title [FX][WIP] Fix handling of attributes [FX] Fix handling of attributes Oct 29, 2020
@Chillee Chillee requested a review from apaszke as a code owner October 29, 2020 18:33
@codecov
Copy link

codecov bot commented Oct 29, 2020

Codecov Report

Merging #47030 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #47030   +/-   ##
=======================================
  Coverage   68.95%   68.95%           
=======================================
  Files         434      434           
  Lines       56207    56220   +13     
=======================================
+ Hits        38760    38769    +9     
- Misses      17447    17451    +4     

Copy link
Collaborator

@jamesr66a jamesr66a left a comment

Choose a reason for hiding this comment

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

Nice

test/test_fx.py Outdated
return self.a.b, self.a.b.t(), self.a.b.view(12)

traced = torch.fx.symbolic_trace(Foo())
assert(all('constant' not in node.name for node in traced.graph.nodes))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh also don't match on name, that isn't stable. You can do this same check on node.target

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@Chillee has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Chillee merged this pull request in cb4b633.

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.

[FX] Methods called on Module parameters are not recorded

3 participants