-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Bits types cannot be used under deterministic mode #109802
Comments
One way could be introducing some special arg torch.empty(..., swear_that_i_fill_it_later_deterministically=True) :) |
We could separate the empty fill feature into its own global setting, so that users can keep it disabled to get better performance. We could add It seems like this feature could potentially be useful for troubleshooting even if determinism is not enabled. If we do this, we should probably mention it on the Reproducibility page. However, I wonder whether fill empty mode should be turned on by default when the user turns on deterministic mode, just to be safe |
I'm not sure to follow what you mean? It already is. We just need to implement fill_empty_deterministic_ for the bits* dtypes. And filling them with 0s sounds like a good default for bits dtypes. |
Yes, it is currently, but I was talking about what potentially should happen if we decided to separate the fill empty feature into its own global setting. There are really two separate issues that Natalia brought up:
I was only addressing the second issue in my earlier comment |
Agree with @kurtamohler, it already is, but I argue it shouldn't be, because non-deterministic empty is not a problem for valid programs that nonetheless want to use deterministic mode, so there's no point in paying this overhead. |
I would still expect that, at least by default, when in deterministic mode, the function returns always the same thing. This is useful for example for testing :D What about:
|
From offline discussion with Natalia, that sounds good. @kurtamohler will you have a bit of time to add that please? |
Sounds good! Yep I can add that |
I am almost ready to submit a PR that adds One idea is to just assert that the empty tensor is not full of NaN/max_int when I modified If uninitialized memory was completely random, then the probability that an int8 tensor of size 10 fails the assert is So I don't think that I think it would be better to somehow check if |
You're calling |
What is LoggingMode? I searched the repo for it and didn't get any results. Are you talking about |
Sorry, LoggingTensorMode, here is how it is used: pytorch/test/test_python_dispatch.py Lines 1089 to 1094 in 733368a
|
That seems like a good idea. I'm not 100% sure how to get it working though. It seems like I'll need to move However, I'm not sure how to make the import torch
def get_logs(fn):
from torch.testing._internal.logging_tensor import LoggingTensorMode, capture_logs
with capture_logs(is_mode=True) as logs:
with LoggingTensorMode():
fn()
return logs
torch.use_deterministic_algorithms(True)
print(get_logs(lambda: torch.empty(10))) I confirmed in gdb that
|
The mode will only capture calls that go through the dispatcher. Calls into the |
Ah I see, that explains why I didn't see the call to |
The toggable warning sounds a bit over-engineered to check that we don't do something. Especially if we don't do anything, it's hard to have code in there to say we're not doing it. I'm personally fine with only testing the deterministic path (like we do today) and manually check that when the flag is disabled we have the expected behavior. |
I'm not sure how to add support for bits types to >>> import torch
>>> torch.empty(4, dtype=torch.bits8)
...
RuntimeError: "_local_scalar_dense_cpu" not implemented for 'Bits8' |
Part of #109802 Pull Request resolved: #111377 Approved by: https://github.com/albanD
…ch#111377) Part of pytorch#109802 Pull Request resolved: pytorch#111377 Approved by: https://github.com/albanD
…ch#111377) Part of pytorch#109802 Pull Request resolved: pytorch#111377 Approved by: https://github.com/albanD
Part of #109802 Pull Request resolved: #111377 Approved by: https://github.com/albanD, https://github.com/aaronenyeshi
…ch#111377) Part of pytorch#109802 Pull Request resolved: pytorch#111377 Approved by: https://github.com/albanD
…ch#111377) Part of pytorch#109802 Pull Request resolved: pytorch#111377 Approved by: https://github.com/albanD, https://github.com/aaronenyeshi
…ch#111377) Part of pytorch#109802 Pull Request resolved: pytorch#111377 Approved by: https://github.com/albanD
…ch#111377) Part of pytorch#109802 Pull Request resolved: pytorch#111377 Approved by: https://github.com/albanD, https://github.com/aaronenyeshi
…ch#111377) Part of pytorch#109802 Pull Request resolved: pytorch#111377 Approved by: https://github.com/albanD, https://github.com/aaronenyeshi
#104995 made it impossible to use bits types under deterministic mode:
produces
RuntimeError: "fill_empty_deterministic_" not implemented for 'Bits8'
While I'm sympathetic to the goals of #104995, I think that a blanket approach forcing filling all empty tensors is too strict. In many cases, determinism mode is set in production runs (because if only deterministic ops are used it imposes very little overhead and provides nice guarantees), and with valid code the empty calls are not a problem. The approach taken in #104995 requires paying filling penalty unconditionally, and also requires implementing fill op for all bits types which pytorch currently doesn't do.
Deterministic empty tensors is a valuable option to have, but in my opinion should not be mixed with existing deterministic mode.
cc @mruberry @kurtamohler
The text was updated successfully, but these errors were encountered: