Skip to content

fix stride compare failed when size value equal to one in ForeachUtils.h#134546

Closed
Shan19900305 wants to merge 1 commit intopytorch:mainfrom
Shan19900305:main_fix__check_tensors_share_sizes_and_strides
Closed

fix stride compare failed when size value equal to one in ForeachUtils.h#134546
Shan19900305 wants to merge 1 commit intopytorch:mainfrom
Shan19900305:main_fix__check_tensors_share_sizes_and_strides

Conversation

@Shan19900305
Copy link
Contributor

@Shan19900305 Shan19900305 commented Aug 27, 2024

When size value equal to one, tensor strides value need be skipped to compare.
@ezyang

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 27, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/134546

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (4 Unrelated Failures)

As of commit c2e38f4 with merge base 7755176 (image):

FLAKY - The following jobs failed but were likely due to flakiness present on trunk:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the release notes: foreach_frontend release notes category label Aug 27, 2024
@zou3519 zou3519 requested a review from janeyx99 August 27, 2024 14:26
@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 27, 2024
@Shan19900305
Copy link
Contributor Author

Copy link
Contributor

@janeyx99 janeyx99 left a comment

Choose a reason for hiding this comment

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

Missing test case!

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this be false?

also isn't left_stride.size() always equal to size_size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, tensor sizes compared before, and here no need to check tensor sizes length.

@Shan19900305 Shan19900305 force-pushed the main_fix__check_tensors_share_sizes_and_strides branch from 7e52980 to b50094c Compare September 2, 2024 06:28
Copy link
Contributor

Choose a reason for hiding this comment

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

The strides for this would be (9, 9, 3, 1) ? => could you comment this for clarity above this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm...this test cases passes for me locally without the change. I haven't figured out why but either this test case is not testing what we want or it is already checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, without ignore dims of one, add op will be called for each input. I will using ForeachFuncWrapper to check this.

@Shan19900305 Shan19900305 force-pushed the main_fix__check_tensors_share_sizes_and_strides branch from 2f0cc88 to 6b0c9c3 Compare September 4, 2024 09:51
@Shan19900305
Copy link
Contributor Author

@janeyx99

@Shan19900305
Copy link
Contributor Author

@janeyx99 Hi, please help review this commit.

@janeyx99
Copy link
Contributor

@pytorchbot merge -r

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Sep 19, 2024
@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

Update aten/src/ATen/native/ForeachUtils.h

Co-authored-by: Jane (Yuan) Xu <31798555+janeyx99@users.noreply.github.com>

Update test/test_foreach.py

Co-authored-by: Jane (Yuan) Xu <31798555+janeyx99@users.noreply.github.com>

Update test/test_foreach.py

Co-authored-by: Jane (Yuan) Xu <31798555+janeyx99@users.noreply.github.com>

Update test/test_foreach.py

Co-authored-by: Jane (Yuan) Xu <31798555+janeyx99@users.noreply.github.com>

Update test/test_foreach.py
@pytorchmergebot
Copy link
Collaborator

Successfully rebased main_fix__check_tensors_share_sizes_and_strides onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout main_fix__check_tensors_share_sizes_and_strides && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the main_fix__check_tensors_share_sizes_and_strides branch from 6b0c9c3 to c2e38f4 Compare September 19, 2024 15:57
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Chao1Han pushed a commit to Chao1Han/pytorch that referenced this pull request Sep 20, 2024
…s.h (pytorch#134546)

When size value equal to one, tensor strides value need be skipped to compare.
@ezyang
Pull Request resolved: pytorch#134546
Approved by: https://github.com/janeyx99
@Shan19900305 Shan19900305 deleted the main_fix__check_tensors_share_sizes_and_strides branch September 23, 2024 09:31
Shan19900305 added a commit to Shan19900305/pytorch_for_push that referenced this pull request Sep 23, 2024
…s.h (pytorch#134546)

When size value equal to one, tensor strides value need be skipped to compare.
@ezyang
Pull Request resolved: pytorch#134546
Approved by: https://github.com/janeyx99
kit1980 pushed a commit that referenced this pull request Sep 25, 2024
…s.h (#136426)

fix stride compare failed when size value equal to one in ForeachUtils.h (#134546)

When size value equal to one, tensor strides value need be skipped to compare.
@ezyang
Pull Request resolved: #134546
Approved by: https://github.com/janeyx99
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: foreach_frontend release notes category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants