-
Notifications
You must be signed in to change notification settings - Fork 561
update gcc #9650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update gcc #9650
Conversation
ysiraichi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
While this fixes #9653, I think that this should be fixed directly inside PyTorch by installing the header (old behavior).
The reason being that we don't depend directly on fmtlib, PyTorch does.
Right, we don't depend on fmtlib, and Pytorch does. However, we are defining the bazel target for PyTorch by specifying a build rule, and the build rule is controlled by us but considers as "Pytorch's build rule". Note that pytorch also have their own bazel target (as they support bazel build: https://github.com/pytorch/pytorch/blob/main/BUILD.bazel) but instead of we depending on that, we created another bazel target for Pytorch by referring to their .so file (which is built by CMake) and headers. |
`fmtlib` version was updated to 12.0.0 in #163441. In this new version, due to fmtlib/fmt#4536, PyTorch started not installing `fmtlib` headers anymore. Because of that, PyTorch/XLA build CI started to fail pytorch/xla#9653. While we did fix it internally pytorch/xla#9650, I believe that PyTorch should continue installing the `fmtlib` headers, since it is a dependency of its C API [`python_arg_parser.h`][1]. PyTorch/XLA CI was moved to `unstable.yml` in #159272, and later removed in #163564. This PyTorch/XLA build failure went under the radar, since the `fmtlib` update only landed on September 22. [1]: https://github.com/pytorch/pytorch/blob/84d673ef577d42d6ec20c6c9f09863583c3111f5/torch/csrc/utils/python_arg_parser.h#L42 Pull Request resolved: #164139 Approved by: https://github.com/Skylion007, https://github.com/malfet
This PR removes the temporary fmtlib dependency introduced in #9650 (see issue #9653). As explained [in this comment][1], the dependency was only added to work around a build issue that should have been fixed upstream in PyTorch. Before the `fmtlib` version bump to 12 in PyTorch (pytorch/pytorch#163441), `fmtlib` headers were exported by default. After the bump, they stopped being exported, which caused the issue. That behavior has since been fixed in PyTorch (pytorch/pytorch#164139), so we can safely remove the explicit fmtlib dependency. [1]: #9650 (review)
No description provided.