[PyTorch] Redirect c10::optional to std::optional by swolchok · Pull Request #101995 · pytorch/pytorch

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]

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

pytorch#1226) Summary: X-link: pytorch/FBGEMM#2138 Landing non-PyTorch portions first; then the PyTorch portions of pytorch/pytorch#101995 will land to Github. Reviewed By: malfet Differential Revision: D51355841

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

pytorch#1226) Summary: X-link: pytorch/FBGEMM#2138 Landing non-PyTorch portions first; then the PyTorch portions of pytorch/pytorch#101995 will land to Github. Reviewed By: malfet Differential Revision: D51355841

#1226) Summary: Pull Request resolved: #1226 X-link: pytorch/FBGEMM#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

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 jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 EikanWang [ghstack-poisoned]

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 jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 EikanWang H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab mrshenli zhaojuanmao rohan-varma kiukchung d4l3k lucasllc XilunWu tianyu-l [ghstack-poisoned]

swolchok · 2023-11-29T02:37:30Z

looks like we're just waiting for the next executorch pinned commit bump

cyyever · 2023-11-29T02:39:56Z

The test failure may be unrelated and can be ignored.

swolchok · 2023-11-29T19:16:57Z

The test failure may be unrelated and can be ignored.

it's definitely related. pytorch/executorch@61aad49 used to be part of this PR but I had to split it out, and we need it pulled in so that this PR won't break the build.

#114717 will update us.

malfet · 2023-11-30T00:54:17Z

@pytorchbot rebase -b main

malfet · 2023-11-30T00:55:05Z

Considering that all other tests were green, we just need to wait for executorch tests to pass to merge this

pytorchmergebot · 2023-11-30T00:55:59Z

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

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 jgong5 mingfeima XiaobingSuper sanchitintel ashokei jingxu10 EikanWang H-Huang awgu kwen2501 wanchaol fegin fduwjj wz337 wconstab mrshenli zhaojuanmao rohan-varma kiukchung d4l3k lucasllc XilunWu tianyu-l [ghstack-poisoned]

pytorchmergebot · 2023-11-30T00:56:16Z

Successfully rebased gh/swolchok/574/orig onto refs/remotes/origin/main, please pull locally before adding more changes (for example, via ghstack checkout https://github.com/pytorch/pytorch/pull/101995)

malfet · 2023-11-30T01:00:56Z

@pytorchbot merge

pytorchmergebot · 2023-11-30T01:02:55Z

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-30T01:41:00Z

@pytorchbot merge

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

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

swolchok added 2 commits November 28, 2023 14:37

This was referenced Nov 30, 2023

Update fbgemm_gpu pin #114847

Closed

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

Closed

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

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;

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!

swolchok commented Nov 29, 2023

Uh oh!

cyyever commented Nov 29, 2023

Uh oh!

swolchok commented Nov 29, 2023

Uh oh!

malfet commented Nov 30, 2023

Uh oh!

malfet commented Nov 30, 2023

Uh oh!

pytorchmergebot commented Nov 30, 2023

Uh oh!

pytorchmergebot commented Nov 30, 2023

Uh oh!

malfet commented Nov 30, 2023

Uh oh!

pytorchmergebot commented Nov 30, 2023

Merge failed

Uh oh!

malfet commented Nov 30, 2023

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!

7 participants

swolchok commented May 22, 2023 •

edited by pytorch-bot bot

Loading

pytorch-bot bot commented May 22, 2023 •

edited

Loading