Conversation
Summary: ## Context See #149164 for more context. Originally, this fix worked but more recently including `cmath` by itself no longer provides access to math constants on Windows platforms. I found that including `math.h` resolves this. I'm not sure exactly what changed, but this PR updates the header to just use both includes fix the symbols not being found. It might be a bug with a recent Windows update perhaps? Test Plan: CI
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/153742
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 5c91450 with merge base cda572b ( UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
math.h for GeLU instead of cmathmath.h for GeLU as well as cmath
swolchok
left a comment
There was a problem hiding this comment.
which constants? all the M_FOO constants? https://learn.microsoft.com/en-us/cpp/c-runtime-library/math-constants?view=msvc-170 says that cmath should be sufficient.
note that as soon as we get C++20 (which I believe will be in June, since the drop-dead date for Ubuntu 20.04 LTS is May 31 per https://ubuntu.com/blog/ubuntu-20-04-lts-end-of-life-standard-support-is-coming-to-an-end-heres-how-to-prepare), we can just migrate to the standard library constants found in https://en.cppreference.com/w/cpp/numeric/constants .
Given that this is an ultratemporary workaround, we can just do this I suppose
|
@pytorchbot merge |
Merge failedReason: This PR needs a If not, please add the To add a label, you can comment to pytorchbot, for example For more information, see Details for Dev Infra teamRaised by workflow job |
Skylion007
left a comment
There was a problem hiding this comment.
May want to add a note that some of these constants are in stdlib in C++20
|
@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 |
Summary:
Context
See #149164 for more context.
Originally, this fix worked but more recently including
cmathby itself no longer provides access to math constants on Windows platforms. I found that includingmath.hresolves this.I'm not sure exactly what changed, but this PR updates the header to just use both includes fix the symbols not being found. It might be a bug with a recent Windows update perhaps?
Test Plan:
CI
cc @jgong5 @mingfeima @XiaobingSuper @sanchitintel @ashokei @jingxu10 @jerryzh168