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

[original author: mrnikwaws] fixing incorrect stride information on xla tensors #5468

Closed
wants to merge 6 commits into from

Conversation

aws-kingrj
Copy link
Collaborator

  • fixing incorrect xla tensor stride information
  • breaks log softmax lowering
  • tested on AWS Neuron internal testing
  • original author mrnikwaws

@JackCaoG
Copy link
Collaborator

Ok I think I know what's going on. The pr from the fork can not use remote cache so build just timeout. Let me see if I can bump the timeout.

@JackCaoG
Copy link
Collaborator

pr in #5470. Let me also grant you write access so you can create branch on pytorch/xla directly. This way all of the CI will use the bazel remote cache and runs much faster.

@JackCaoG
Copy link
Collaborator

@aws-kingrj if you rebase the build should start run pass timeout.

@@ -161,6 +161,11 @@ at::IntArrayRef XLATensorImpl::strides_custom() const {
return strides_default();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mentioned this breaks log softmax lowering. Can you add a test that will fail without this change? The change in general make sense but without a test it is hard to make sure this is correct nor can we prevent this from regressing again.

@@ -3700,6 +3700,19 @@ TEST_F(AtenXlaTensorTest, TestPowIntExponent) {
ExpectCounterNotChanged("aten::.*", cpp_test::GetIgnoredCounters());
}

TEST_F(AtenXlaTensorTest, TestPowFloatScalarBaseIntExponent) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the commit is a bit messed up, this seems to be from other pr

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