Skip to content

Conversation

@ailzhang
Copy link
Contributor

@ailzhang ailzhang commented Jul 5, 2019

  • Refactor all uses of at:: with torch:: APIs in test_aten_xla_tensor.cpp, basically this file shouldn’t touch any at:: and torch_xla::/bridge internals, instead only torch:: APIs are allowed there. Other files are used to test internals specifically, so at::/torch_xla/bridge are fine there.
  • Refactor a few test helper functions accordingly, make sure all comparison happens on CPU tensor.
  • Refactor test CMakeList to rely include Torch package instead of linking each .so file separately. (hard to maintain).
  • Remove torch_xla::ToTensor(at::Tensor) which was used to extract Tensor out of a Variable. Upstream was fixed by Use at::AutoNonVariableTypeMode before calling ATen tensor factory functions pytorch#22364 so that we make sure all tensors passed to xla has tensor.is_variable()=false. We can safely remove this hack now.
  • After this PR, when a new op lowering is added, we should add tests in test_aten_xla_tensor.cpp and test_tensor.cpp for it.
    Note that this PR is not clang-formatted yet, we need to find a open-source based clang-format config :P (otherwise it's not convenient to ask every OSS contributor to have clang-format-9 installed from source).
    Detailed context: https://docs.google.com/document/d/1nkiPPhcQy6RpBQ13Y-HGkuefnY-B-3kgFveQ0h85Jjo/edit#

@ailzhang ailzhang requested review from asuhan, dlibenzi, gchanan and yf225 July 5, 2019 22:29
EXPECT_TRUE(CloseValues(tensor, xla_tensor.ToTensor(), rtol, atol));
}

void ForEachDevice(const std::function<void(const Device&)>& devfn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this method? I'd try to remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't, test_tensor.cpp still uses it.

@dlibenzi
Copy link
Collaborator

dlibenzi commented Jul 6, 2019

@ailzhang Please, the next time send the document first, let's discuss it, and the you do the code ;)
With a 6+K LOC PR, most of which a search&replace, but also bundles other stuff, it is better to split the big search&replace part into one PR (which can be LGTM-ed in one second), and the "other stuff" which requires more thinking into a separate one.
This prevents the reviewers in going through 6K LOC picking up search&replace parts, for the things which need more attention.

EXPECT_TRUE(CloseValues(tensor, xla_tensor.ToTensor(), rtol, atol));
}

void ForEachDevice(const std::function<void(const Device&)>& devfn);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't, test_tensor.cpp still uses it.


void ForEachDevice(const std::function<void(const Device&)>& devfn);

void ForEachTorchXLADevice(const std::function<void(const torch::Device&)>& devfn);
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to have different name, ForEachDevice() should be fine.

@ailzhang
Copy link
Contributor Author

ailzhang commented Jul 7, 2019

FYI I've run this PR manually against the bert model and it stabilizes at 3 graphs.

Copy link
Collaborator

@dlibenzi dlibenzi left a comment

Choose a reason for hiding this comment

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

Approval on the basis CPU and TPU tests are green with this.

@ailzhang ailzhang merged commit ef4ff9f into pytorch:master Jul 8, 2019
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.

3 participants