pytorch: renamed from libtorch #111889

singingwolfboy · 2022-09-28T09:55:59Z

Have you followed the guidelines for contributing?
Have you ensured that your commits follow the commit style guide?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This renames libtorch to pytorch, and installs the Python module as well as the C++ libraries. This was originally #110199, but I opened a new pull request to get more attention on this change.

carlocab · 2022-09-30T01:14:41Z

Formula/pytorch.rb

+  depends_on "pyyaml"
+
+  on_macos do
+    depends_on arch: :arm64


I'm fine with the rename, but I'm not fine with making this depend on ARM.

If it fails to build on Intel, then we need to understand why and fix it. Or, at least, report the problem upstream so that it can be fixed there.

I just reported the problem upstream: pytorch/pytorch#85956

danielnachun · 2022-10-06T01:00:17Z

@singingwolfboy when #110116 is merged could you rebase this? It should now work on Intel macOS so you won't need to have the ARM requirement any more. That was the only blocker we seemed to have over this.

I added some of your changes in that PR as well so you shouldn't have to do much other than the rename itself and tweaks you made to the build parameters and test.

singingwolfboy · 2022-10-06T21:15:43Z

Ugh, still failing! 😩 This is the new error message, still only on Intel Macs:

error: unknown warning option '-Wno-unused-but-set-variable'; did you mean '-Wno-unused-const-variable'? [-Werror,-Wunknown-warning-option]

Does anyone know what might be causing this, or how to fix it?

danielnachun · 2022-10-07T01:15:38Z

Ugh, still failing! 😩 This is the new error message, still only on Intel Macs:
error: unknown warning option '-Wno-unused-but-set-variable'; did you mean '-Wno-unused-const-variable'? [-Werror,-Wunknown-warning-option]
Does anyone know what might be causing this, or how to fix it?

That's odd because we literally just built this with no issues and #112520 seems to have gotten past building fbgemm at least, because it timed out after 90 minutes. It might be worth merging that PR first and rebasing here again to minimize the changes you are making to the libtorch formula aside from the renaming itself. That can help us narrow down why that flag is erroneously being used when it doesn't exist.

danielnachun · 2022-10-07T18:40:07Z

Formula/pytorch.rb

+    inreplace "cmake/Dependencies.cmake",
+      'if("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang" AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 13.0.0)',
+      'if("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")'
+


I think instead we need to backport the part of pytorch/pytorch@d7e6aaa that changes

target_compile_options(asmjit -Wno-unused-but-set-variable)

to

target_compile_options_if_supported(asmjit -Wno-unused-but-set-variable)

That will make sure that flag is only added if the compile supports the flag, where as in the current release it uses the flag unconditionally.

I thought about doing that, but unfortunately the target_compile_options_if_supported macro is not present in the 1.12.1 release! It's a custom macro that was committed after the release was created. So we would have to patch that function back in as well, which seems like a bigger change than necessary.

I think maybe we can just delete these lines entirely? That's what I'm trying now in my latest commit.

Nope, patches aren't allowed in new formulae, and brew-test-bot sees this as a new formula (even though it's a rename, which is different). So I'm going to try patching it to if(FALSE) instead for the same effect.

It looks like this worked now and just timed out, though we should only do the inreplace if the macOS version is less than or equal to Catalina.

Why less than or equal to Catalina? Also, could you please add the "long build" label to this pull request, so that it doesn't time out for the next CI build?

Actually let's do a CI run to see if all platforms can build with the if(FALSE) fix used. We'll be able to drop it in the next release anyway.

CI worked at last! Great job hunting down that fix, and thanks for your patience. This PR was beset by multiple unrelated problems that were non-trivial to fix but I think we're ready to go. I will merge this tomorrow if there has been no other maintainer feedback as the other changes here are pretty straight forward.

danielnachun · 2022-10-08T22:37:19Z

Formula/pytorch.rb

+
+    inreplace "torch/utils/cpp_extension.py", "_TORCH_PATH = os.path.dirname(os.path.dirname(_HERE))",
+                                              "_TORCH_PATH = \"#{opt_prefix}\""
+


I don't think it should block this PR, but I think we should report upstream that there should be a way to set the paths to torch_shm_manager and _TORCH_PATH as CMake options. Hardcoding them like this reduces the portability of this package and these types of paths are usually pretty easy to pass in as options.

fxcoudert · 2022-10-09T13:38:19Z

Thank you @singingwolfboy for this pull request!

BrewTestBot · 2022-10-09T13:41:18Z

@fxcoudert has triggered a merge.

BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Sep 28, 2022

singingwolfboy force-pushed the libtorch-to-pytorch branch from 344ca50 to a126fca Compare September 29, 2022 12:45

singingwolfboy mentioned this pull request Sep 29, 2022

torchvision: install Python module #111986

Closed

6 tasks

carlocab reviewed Sep 30, 2022

View reviewed changes

singingwolfboy force-pushed the libtorch-to-pytorch branch 5 times, most recently from f187218 to ef62fa5 Compare October 3, 2022 12:30

miles170 mentioned this pull request Oct 3, 2022

libtorch: fix python3 reference. #110116

Merged

6 tasks

singingwolfboy force-pushed the libtorch-to-pytorch branch from ef62fa5 to 7838297 Compare October 3, 2022 14:08

singingwolfboy mentioned this pull request Oct 3, 2022

Pytorch fails to compile on Intel Mac pytorch/pytorch#85956

Closed

singingwolfboy force-pushed the libtorch-to-pytorch branch 3 times, most recently from e26560c to b0965ed Compare October 6, 2022 17:13

singingwolfboy mentioned this pull request Oct 6, 2022

libtorch: enable more features #112520

Closed

singingwolfboy force-pushed the libtorch-to-pytorch branch from b0965ed to d077c18 Compare October 7, 2022 10:10

danielnachun reviewed Oct 7, 2022

View reviewed changes

singingwolfboy force-pushed the libtorch-to-pytorch branch from 1dc991b to e02b660 Compare October 7, 2022 21:29

singingwolfboy added 2 commits October 8, 2022 13:57

pytorch: renamed from libtorch

b23e8d5

torchvision: depend on pytorch

c1bdd38

singingwolfboy force-pushed the libtorch-to-pytorch branch from e02b660 to c1bdd38 Compare October 8, 2022 11:57

danielnachun added long build Set a long timeout for formula testing CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. labels Oct 8, 2022

danielnachun reviewed Oct 8, 2022

View reviewed changes

danielnachun removed the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Oct 8, 2022

fxcoudert approved these changes Oct 9, 2022

View reviewed changes

BrewTestBot closed this in e9dabcf Oct 9, 2022

github-actions bot added the outdated PR was locked due to age label Nov 9, 2022

github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pytorch: renamed from libtorch #111889

pytorch: renamed from libtorch #111889

singingwolfboy commented Sep 28, 2022 •

edited

Loading

carlocab Sep 30, 2022

singingwolfboy Sep 30, 2022

carlocab Sep 30, 2022

danielnachun commented Oct 6, 2022

singingwolfboy commented Oct 6, 2022

danielnachun commented Oct 7, 2022

danielnachun Oct 7, 2022

singingwolfboy Oct 7, 2022

singingwolfboy Oct 7, 2022

danielnachun Oct 8, 2022

singingwolfboy Oct 8, 2022

danielnachun Oct 8, 2022

danielnachun Oct 8, 2022

danielnachun Oct 8, 2022

fxcoudert commented Oct 9, 2022

BrewTestBot commented Oct 9, 2022


		inreplace "torch/utils/cpp_extension.py", "_TORCH_PATH = os.path.dirname(os.path.dirname(_HERE))",
		"_TORCH_PATH = \"#{opt_prefix}\""

pytorch: renamed from libtorch #111889

pytorch: renamed from libtorch #111889

Conversation

singingwolfboy commented Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielnachun commented Oct 6, 2022

singingwolfboy commented Oct 6, 2022

danielnachun commented Oct 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fxcoudert commented Oct 9, 2022

BrewTestBot commented Oct 9, 2022

singingwolfboy commented Sep 28, 2022 •

edited

Loading