Move at::{Refcounted,}MapAllocator to c10 #109881

peterbell10 · 2023-09-22T14:23:07Z

Stack from ghstack (oldest at bottom):

-> Move at::{Refcounted,}MapAllocator to c10 #109881

libshm.so depends on the torch library exclusively for at::RefcountedMapAllocator,
so it makes sense to move it to c10 along with the other memory allocators.

This means libshm.so only depends on c10 and we don't need to relink
libshm.so for every ATen change.

libshm.so depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. [ghstack-poisoned]

pytorch-bot · 2023-09-22T14:23:10Z

🔗 Helpful Links

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

📄 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.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

MacOS outage (2023-10-13)

✅ No Failures

As of commit 2e1530c with merge base 625a3b1 ():
💚 Looks good so far! There are no failures yet. 💚

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

`libshm.so` depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. [ghstack-poisoned]

peterbell10 · 2023-10-04T13:41:24Z

c10/core/MapAllocator.h

+
+C10_API std::string NewProcessWideShmHandle();
+
+class C10_API MapAllocator {


These diffs are a mess, but it's mostly just a copy paste of the aten files with aten changed to c10. However, with the linter changes, it's too big of a diff for git to recognize as a simple rename. This is clearer in the commit-level diffs.

albanD

Ho very nice cleanup!

I'll verify that all build are good and then merge this.

albanD · 2023-10-04T17:43:52Z

@pytorchbot rebase

pytorchmergebot · 2023-10-04T17:45:58Z

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

`libshm.so` depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. [ghstack-poisoned]

pytorchmergebot · 2023-10-04T17:46:15Z

Successfully rebased gh/peterbell10/619/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/109881)

libshm.so depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. ghstack-source-id: 198c9ee Pull Request resolved: #109881

albanD · 2023-10-09T17:41:35Z

I can't import this so yolo

@pytorchbot merge -r

pytorchmergebot · 2023-10-09T17:43:36Z

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

`libshm.so` depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. [ghstack-poisoned]

pytorchmergebot · 2023-10-09T17:43:49Z

Successfully rebased gh/peterbell10/619/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/109881)

libshm.so depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. ghstack-source-id: 7b78056 Pull Request resolved: #109881

peterbell10 · 2023-10-12T10:48:27Z

@pytorchbot merge

pytorchmergebot · 2023-10-12T10:50:38Z

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

kit1980 · 2023-10-13T17:56:01Z

@pytorchbot revert -m "breaking internal builds, undefined symbol: _ZN3c1022RefcountedMapAllocator6decrefEv" -c ghfirst

kit1980 · 2023-10-13T17:56:56Z

@peterbell10 You probably need someone from Meta to help re-landing this.

pytorchmergebot · 2023-10-13T17:57:47Z

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

pytorchmergebot · 2023-10-13T17:57:57Z

@peterbell10 your PR has been successfully reverted.

This reverts commit 68a1219. Reverted #109881 on behalf of https://github.com/kit1980 due to breaking internal builds, undefined symbol: _ZN3c1022RefcountedMapAllocator6decrefEv ([comment](#109881 (comment)))

peterbell10 · 2023-10-13T18:26:23Z

c10/BUCK.oss

-    preprocessor_flags = ['-DC10_BUILD_MAIN_LIB'],
+    preprocessor_flags = [
+        '-DC10_BUILD_MAIN_LIB',
+        '-DHAVE_MMAP=1',


undefined symbol: _ZN3c1022RefcountedMapAllocator6decrefEv"

I had the same error in the bazel build when HAVE_MMAP wasn't being defined. Is it possible there is another build definition that needs the flag added?

If so, does that mean we have 4 completely unrelated build definitions: CMake, bazel, buck (oss), buck (internal)?

@peterbell10 unfortunately it's even more than 4: more than 1 internal buck.

But they are not completely unrelated: all of them are supposed to use build.bzl files (like https://github.com/pytorch/pytorch/blob/main/c10/build.bzl), but it's only implemented for some things so far.

For this particular case, likely https://github.com/pytorch/pytorch/blob/main/c10/ovrsource_defs.bzl needs to be updated. But someone from Meta should work with you on this.

`libshm.so` depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. [ghstack-poisoned]

libshm.so depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. ghstack-source-id: 96dea8e Pull Request resolved: #109881

`libshm.so` depends on the torch library exclusively for `at::RefcountedMapAllocator`, so it makes sense to move it to c10 along with the other memory allocators. This means `libshm.so` only depends on `c10` and we don't need to relink `libshm.so` for every ATen change. [ghstack-poisoned]

peterbell10 · 2023-10-13T20:46:11Z

c10/ovrsource_defs.bzl

-        deps = select({
+        deps = [
+            "//third-party/rt",
+        ] + select({


@kit1980 would you be able to help on the meta-internal side? I think this might fix it but not idea if it works.

I can try but not today.

@kit1980 any updates on this?

ping @kit1980

I'll look today.

github-actions · 2023-12-13T03:33:55Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

kit1980 · 2023-12-20T19:26:07Z

@pytorchbot rebase

pytorchmergebot · 2023-12-20T19:27:53Z

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

pytorchmergebot · 2023-12-20T19:27:57Z

Rebase failed due to Command git -C /home/runner/work/pytorch/pytorch rebase refs/remotes/origin/viable/strict gh/peterbell10/619/orig returned non-zero exit code 1

Rebasing (1/1)
Auto-merging BUILD.bazel
Auto-merging aten/src/ATen/CMakeLists.txt
CONFLICT (modify/delete): aten/src/ATen/MapAllocator.cpp deleted in 11e28cb2f63 (Move at::{Refcounted,}MapAllocator to c10) and modified in HEAD.  Version HEAD of aten/src/ATen/MapAllocator.cpp left in tree.
Auto-merging build_variables.bzl
Auto-merging c10/CMakeLists.txt
Auto-merging c10/ovrsource_defs.bzl
Auto-merging caffe2/CMakeLists.txt
Auto-merging cmake/Codegen.cmake
error: could not apply 11e28cb2f63... Move at::{Refcounted,}MapAllocator to c10
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 11e28cb2f63... Move at::{Refcounted,}MapAllocator to c10

Raised by https://github.com/pytorch/pytorch/actions/runs/7279909983

kit1980 · 2023-12-20T19:33:09Z

@peterbell10 May I ask you to fix the conflicts?
Or even better create a new non-ghstack PR - it will simplify things for me (it's a single PR stack anyway).

peterbell10 · 2023-12-20T20:17:51Z

Okay, rebased and resubmitted as #116204

peterbell10 mentioned this pull request Sep 22, 2023

[Easy] Fix "comparison of unsigned integer with zero" warning #109882

Closed

pytorchbot added the open source label Sep 22, 2023

peterbell10 added 6 commits September 22, 2023 18:12

peterbell10 added the ciflow/periodic Trigger jobs ran periodically on master (periodic.yml) on the PR label Oct 4, 2023

peterbell10 marked this pull request as ready for review October 4, 2023 13:39

peterbell10 requested a review from albanD October 4, 2023 13:39

peterbell10 commented Oct 4, 2023

View reviewed changes

albanD approved these changes Oct 4, 2023

View reviewed changes

peterbell10 added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 5, 2023

pytorchmergebot added the merging label Oct 12, 2023

pytorchmergebot removed the merging label Oct 12, 2023

pytorchmergebot closed this in 68a1219 Oct 12, 2023

peterbell10 reopened this Oct 13, 2023

peterbell10 commented Oct 13, 2023

View reviewed changes

github-actions bot added the Stale label Dec 13, 2023

peterbell10 removed the Stale label Dec 13, 2023

peterbell10 mentioned this pull request Dec 20, 2023

[RELAND] Move at::{Refcounted,}MapAllocator to c10 #116204

Closed

peterbell10 closed this Dec 20, 2023

facebook-github-bot deleted the gh/peterbell10/619/head branch December 24, 2023 15:24


		C10_API std::string NewProcessWideShmHandle();

		class C10_API MapAllocator {

Move at::{Refcounted,}MapAllocator to c10 #109881

Move at::{Refcounted,}MapAllocator to c10 #109881

Uh oh!

Conversation

peterbell10 commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Sep 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/109881

❗ 1 Active SEVs

✅ No Failures

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albanD left a comment

Choose a reason for hiding this comment

Uh oh!

albanD commented Oct 4, 2023

Uh oh!

pytorchmergebot commented Oct 4, 2023

Uh oh!

pytorchmergebot commented Oct 4, 2023

Uh oh!

albanD commented Oct 9, 2023

Uh oh!

pytorchmergebot commented Oct 9, 2023

Uh oh!

pytorchmergebot commented Oct 9, 2023

Uh oh!

peterbell10 commented Oct 12, 2023

Uh oh!

pytorchmergebot commented Oct 12, 2023

Merge started

Uh oh!

kit1980 commented Oct 13, 2023

Uh oh!

kit1980 commented Oct 13, 2023

Uh oh!

pytorchmergebot commented Oct 13, 2023

Uh oh!

pytorchmergebot commented Oct 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 13, 2023

Uh oh!

kit1980 commented Dec 20, 2023

Uh oh!

pytorchmergebot commented Dec 20, 2023

Uh oh!

pytorchmergebot commented Dec 20, 2023

Uh oh!

kit1980 commented Dec 20, 2023

Uh oh!

peterbell10 commented Dec 20, 2023

Uh oh!

Uh oh!

peterbell10 commented Sep 22, 2023 •

edited

Loading

pytorch-bot bot commented Sep 22, 2023 •

edited

Loading