Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/164139
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 7cef2e0 with merge base 90512fa ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
I don't think we want to install libfmt, otherwise we could have used it in more headers. libfmt have never been installed. Is it better to remove usage of it from |
|
If I understand it correctly, it was installed by default before fmtlib/fmt#4536, since we didn't explicitly set |
I don't think that solves the issue. |
|
Yeah it should be installed and installing it was the default it. Note: @cyyever in this case installing it just means including it in then headers included with the PyTorch wheel. |
|
@pytorchbot merge |
malfet
left a comment
There was a problem hiding this comment.
This is probably fine, though I wonder if we really want to ship fmtlib headers with torch wheel
Merge startedYour 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 |
We do if want users to be able to build torch cpp_extensions etc. Don't we already ship a ton of other headers from third_party packages already in the include path? It's either that or ban libfmt from all header files in PyTorch. |
|
The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command |
|
@pytorchbot merge |
Merge startedYour 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 |
|
@pytorchbot merge -f "ROCM pending" |
|
The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
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)
fmtlibversion was updated to 12.0.0 in #163441.In this new version, due to fmtlib/fmt#4536, PyTorch started not installing
fmtlibheaders 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 thefmtlibheaders, since it is a dependency of its C APIpython_arg_parser.h.PyTorch/XLA CI was moved to
unstable.ymlin #159272, and later removed in #163564. This PyTorch/XLA build failure went under the radar, since thefmtlibupdate only landed on September 22.