[PyTorch] Redirect c10::optional to std::optional #101995

swolchok · 2023-05-22T18:32:42Z

Stack from ghstack (oldest at bottom):

-> [PyTorch] Redirect c10::optional to std::optional #101995

We have C++17 now!

I am intentionally dropping the c10::optional<c10::ArrayRef> size optimization. It was intended to improve dispatch, but thanks to D34602980 / #70864 we don't use optional<ArrayRef> in function arguments anymore anyway.

Differential Revision: D46079028

cc @mrshenli @pritamdamania87 @zhaojuanmao @satgera @rohan-varma @gqchen @aazzolini @osalpekar @jiayisuse @H-Huang @kwen2501 @awgu @penguinwu @fegin @XilunWu @wanchaol @fduwjj @wz337 @tianyu-l @wconstab @yf225 @ezyang @gchanan @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10 @EikanWang @kiukchung @d4l3k @LucasLLC

We have C++17 now! I am intentionally dropping the c10::optional<c10::ArrayRef> size optimization. It was intended to improve dispatch, but thanks to D34602980 / #70864 we don't use optional<ArrayRef> in function arguments anymore anyway. Differential Revision: [D46079028](https://our.internmc.facebook.com/intern/diff/D46079028/) [ghstack-poisoned]

pytorch-bot · 2023-05-22T18:32:45Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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

✅ You can merge normally! (1 Unrelated Failure)

As of commit ac98e66 with merge base 597d3fb ():

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

pull / linux-focal-py3_8-clang9-xla / test (xla, 1, 1, linux.12xlarge) (gh)

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

We have C++17 now! I am intentionally dropping the c10::optional<c10::ArrayRef> size optimization. It was intended to improve dispatch, but thanks to D34602980 / #70864 we don't use optional<ArrayRef> in function arguments anymore anyway. Differential Revision: [D46079028](https://our.internmc.facebook.com/intern/diff/D46079028/) [ghstack-poisoned]

Pull Request resolved: #101995 We have C++17 now! I am intentionally dropping the c10::optional<c10::ArrayRef> size optimization. It was intended to improve dispatch, but thanks to D34602980 / #70864 we don't use optional<ArrayRef> in function arguments anymore anyway. ghstack-source-id: 189899247 Differential Revision: [D46079028](https://our.internmc.facebook.com/intern/diff/D46079028/)

albanD

Code change sounds fine, will let @malfet take the final look!

albanD · 2023-05-22T20:33:19Z

c10/test/util/optional_test.cpp

    // Non-trivial destructor.
    std::string>;

-// This assert is also in Optional.cpp; including here too to make it


cc @ezyang if you have more context about this

This is @swolchok's optimization, he knows ;)

malfet · 2023-05-22T20:40:07Z

aten/src/ATen/core/ATen_fwd.h

-template<typename T>
-class optional;


Should we just say something like using optional = std::optional; and kill c10/util/Optional header?

that'd be fine with me, but I've had to leave in #include <c10/macros/Macros.h> in c10/util/Optional.h for now because lots of files seem to depend on pulling in the Macros.h namespace shenanigans through Optional.h :(

Agre with @malfet . This is how llvm handled their std::optional conversion.

We have C++17 now! I am intentionally dropping the c10::optional<c10::ArrayRef> size optimization. It was intended to improve dispatch, but thanks to D34602980 / #70864 we don't use optional<ArrayRef> in function arguments anymore anyway. Differential Revision: [D46079028](https://our.internmc.facebook.com/intern/diff/D46079028/) cc ezyang gchanan EikanWang jgong5 [ghstack-poisoned]

…d::optional doesn't pass them on "[PyTorch] Redirect c10::optional to std::optional" We have C++17 now! I am intentionally dropping the c10::optional<c10::ArrayRef> size optimization. It was intended to improve dispatch, but thanks to D34602980 / #70864 we don't use optional<ArrayRef> in function arguments anymore anyway. Differential Revision: [D46079028](https://our.internmc.facebook.com/intern/diff/D46079028/) cc ezyang gchanan EikanWang jgong5 [ghstack-poisoned]

pytorchmergebot · 2023-11-30T01:42:51Z

Merge failed

Reason: This PR has internal changes and must be landed via Phabricator

Details for Dev Infra team

Raised by workflow job

malfet · 2023-11-30T02:06:34Z

@pytorchbot merge

pytorchmergebot · 2023-11-30T02:08:33Z

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

malfet · 2023-11-30T02:44:22Z

@pytorchbot merge -f "Before rebase everything was green but executorch, see https://hud.pytorch.org/pytorch/pytorch/pull/101995?sha=18faa09012ed31baa8c37cce184dcbe36202d9ba "

pytorchmergebot · 2023-11-30T02:44:42Z

The merge job was canceled. If you believe this is a mistake, then you can re trigger it through pytorch-bot.

pytorchmergebot · 2023-11-30T02:46:33Z

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

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

Should have been landed together with #101995

Should have been landed together with #101995 Includes pytorch/FBGEMM@de731af Pull Request resolved: #114847 Approved by: https://github.com/kit1980, https://github.com/huydhn

We have C++17 now! I am intentionally dropping the `c10::optional<c10::ArrayRef>` size optimization. It was intended to improve dispatch, but thanks to D34602980 / pytorch#70864 we don't use `optional<ArrayRef>` in function arguments anymore anyway. Differential Revision: [D46079028](https://our.internmc.facebook.com/intern/diff/D46079028/) Pull Request resolved: pytorch#101995 Approved by: https://github.com/malfet, https://github.com/Skylion007, https://github.com/ezyang

Should have been landed together with pytorch#101995 Includes pytorch/FBGEMM@de731af Pull Request resolved: pytorch#114847 Approved by: https://github.com/kit1980, https://github.com/huydhn

pytorch#2138) Summary: X-link: pytorch/executorch#1226 Pull Request resolved: pytorch#2138 Landing non-PyTorch portions first; then the PyTorch portions of pytorch/pytorch#101995 will land to Github. Reviewed By: malfet Differential Revision: D51355841 fbshipit-source-id: 4eed885733189f21e342613431b637de72979cb4

…1995)" This reverts commit 165f4f6.

swolchok requested review from H-Huang, awgu, d4l3k, fegin, kiukchung, kwen2501, mrshenli, rohan-varma, wanchaol and zhaojuanmao as code owners May 22, 2023 20:12

github-actions bot added the NNC label May 22, 2023

pytorch-bot bot added the release notes: distributed (c10d) release notes category label May 22, 2023

albanD reviewed May 22, 2023

View reviewed changes

malfet approved these changes May 22, 2023

View reviewed changes

malfet added the topic: bc breaking topic category label May 22, 2023

albanD added the module: bc-breaking Related to a BC-breaking change label May 22, 2023

malfet reviewed May 22, 2023

View reviewed changes

swolchok requested review from IvanYashchuk, lezcano and nikitaved as code owners May 23, 2023 18:29

pytorchmergebot removed the merging label Nov 30, 2023

pytorchmergebot added the merging label Nov 30, 2023

pytorchmergebot added the Merged label Nov 30, 2023

pytorchmergebot removed the merging label Nov 30, 2023

pytorchmergebot closed this in 165f4f6 Nov 30, 2023

malfet added a commit that referenced this pull request Nov 30, 2023

Update fbgemm_gpu pin

c433bb9

Should have been landed together with #101995

malfet mentioned this pull request Nov 30, 2023

Update fbgemm_gpu pin #114847

Closed

facebook-github-bot deleted the gh/swolchok/574/head branch December 3, 2023 15:27

malfet mentioned this pull request Dec 5, 2023

Disable bugprone-unchecked-optional-access as it can cause clang-tidy to hang #115124

Closed

albanD added oncall: distributed Add this issue/PR to distributed oncall triage queue and removed module: distributed labels Dec 8, 2023

malfet mentioned this pull request Mar 6, 2024

CUDA Extension Bug for Pytorch 2.2.0 #118842

Open

michael-sandoval mentioned this pull request May 8, 2024

Error after compiling with ROCM #119849

Closed

michael-sandoval added a commit to michael-sandoval/pytorch that referenced this pull request Jul 20, 2024

Revert "[PyTorch] Redirect c10::optional to std::optional (pytorch#10…

be42f03

…1995)" This reverts commit 165f4f6.

akihironitta mentioned this pull request Feb 12, 2025

Replace c10::optional with equivalent std::optional pyg-team/pyg-lib#406

Merged

swolchok mentioned this pull request Oct 10, 2025

Refactor out headeronly ArrayRef #164991

Closed

		template<typename T>
		class optional;

[PyTorch] Redirect c10::optional to std::optional #101995

[PyTorch] Redirect c10::optional to std::optional #101995

Uh oh!

Conversation

swolchok commented May 22, 2023 • edited by pytorch-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ You can merge normally! (1 Unrelated Failure)

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD May 22, 2023

Choose a reason for hiding this comment

Uh oh!

ezyang May 23, 2023

Choose a reason for hiding this comment

Uh oh!

malfet May 22, 2023

Choose a reason for hiding this comment

Uh oh!

swolchok May 23, 2023

Choose a reason for hiding this comment

Uh oh!

Skylion007 May 24, 2023

Choose a reason for hiding this comment

Uh oh!

pytorchmergebot commented Nov 30, 2023

Merge failed

Uh oh!

malfet commented Nov 30, 2023

Uh oh!

pytorchmergebot commented Nov 30, 2023

Merge started

Uh oh!

malfet commented Nov 30, 2023

Uh oh!

pytorchmergebot commented Nov 30, 2023

Uh oh!

pytorchmergebot commented Nov 30, 2023

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

swolchok commented May 22, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented May 22, 2023 •

edited

Loading