Skip to content

Conversation

@peterbell10
Copy link
Collaborator

@peterbell10 peterbell10 commented May 2, 2023

Stack from ghstack (oldest at bottom):

AT_USE_JITERATOR evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

@pytorch-bot
Copy link

pytorch-bot bot commented May 2, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit 065ee34:
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the release notes: cuda release notes category label May 2, 2023
peterbell10 added a commit that referenced this pull request May 2, 2023
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

ghstack-source-id: be1e006
Pull Request resolved: #100464
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

[ghstack-poisoned]
peterbell10 added a commit to peterbell10/pytorch that referenced this pull request May 3, 2023
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

ghstack-source-id: be1e006
Pull Request resolved: pytorch#100464
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

[ghstack-poisoned]
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

[ghstack-poisoned]
@peterbell10 peterbell10 added the topic: not user facing topic category label May 3, 2023
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

[ghstack-poisoned]
peterbell10 added a commit that referenced this pull request May 3, 2023
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

ghstack-source-id: 8018036
Pull Request resolved: #100464
@peterbell10 peterbell10 added the keep-going Don't stop on first failure, keep running tests until the end label May 3, 2023
peterbell10 added a commit that referenced this pull request May 3, 2023
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

ghstack-source-id: 09fade4
Pull Request resolved: #100464
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

[ghstack-poisoned]
@peterbell10 peterbell10 marked this pull request as ready for review May 4, 2023 15:22
@peterbell10 peterbell10 requested a review from lezcano May 5, 2023 21:57
namespace at::native {

CONSTEXPR_EXCEPT_WIN_CUDA char acos_name[] = "acos";
CONSTEXPR_EXCEPT_WIN_CUDA char acos_name[] = "acos_impl";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The _impl is necessary because otherwise it calls acos defined in the jiterator headers which fails because the template parameters are different. Same issue exists for all of these.

auto common_dtype = iter.common_dtype();
if (at::isComplexType(common_dtype)) {
#if AT_USE_JITERATOR
// Disabled due to accuracy issues
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some of these might just need tolerance bumps, but I didn't want to spend much time on this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you open an issue describing what would need doing and tag it as "good first issue"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened #100842

Copy link
Collaborator

@lezcano lezcano left a comment

Choose a reason for hiding this comment

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

Good catch!

Is there any way we could have a regression test for this (even if it's xfailed for now, given that all these are deactivated now).

Also cc @kshitij12345 as he probably wrote a few of these.

auto common_dtype = iter.common_dtype();
if (at::isComplexType(common_dtype)) {
#if AT_USE_JITERATOR
// Disabled due to accuracy issues
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you open an issue describing what would need doing and tag it as "good first issue"?

@peterbell10
Copy link
Collaborator Author

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a rebase job. Check the current status here

`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

[ghstack-poisoned]
@pytorchmergebot
Copy link
Collaborator

Successfully rebased gh/peterbell10/545/orig onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/100464)

pytorchmergebot pushed a commit that referenced this pull request May 7, 2023
`AT_USE_JITERATOR` evaluates to false when the definition isn't
included, so these files were not using jiterator at all.

ghstack-source-id: 762d30b
Pull Request resolved: #100464
@peterbell10 peterbell10 added the ciflow/trunk Trigger trunk jobs on your pull request label May 7, 2023
@peterbell10
Copy link
Collaborator Author

@pytorchbot merge

@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

Copy link
Collaborator

@ngimel ngimel left a comment

Choose a reason for hiding this comment

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

large/extreme values could have bugs in the implementation, libc++ might not handle those cases correctly (iirc we had to skip some mac tests for extreme values for the same reason)

@facebook-github-bot facebook-github-bot deleted the gh/peterbell10/545/head branch June 8, 2023 18:27
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 keep-going Don't stop on first failure, keep running tests until the end Merged open source release notes: cuda release notes category topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants