Skip to content
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

Set preserve_rng_state to True if there is dropout #702

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

chrisc36
Copy link
Contributor

This is consistent with the documentation which states it should be true when deterministic random behavior is required.

…nverse

This is consistent with the documentation which states it should be true when deterministic random behavior is required.
@chrisc36 chrisc36 requested a review from dirkgr August 13, 2024 22:40
Copy link
Member

@dirkgr dirkgr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I would love to hear from @epwalsh , just in case both @chrisc36 and I are confused about how this works?

Copy link
Member

@epwalsh epwalsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is correct. The documentation is a little confusing though. The official description of this parameter is worded like it has the opposite effect:
image

That might explain why we had it the way we did.

@2015aroras
Copy link
Collaborator

Can't we just set this to True? Unless this is demonstrably harmful to have enabled when dropout is off, it would be nice to have this set to true so it cannot somehow become a determinism issue later.

@epwalsh
Copy link
Member

epwalsh commented Aug 14, 2024

We might get a little perf back when it's false.

@chrisc36 chrisc36 merged commit 6c4d53f into allenai:main Aug 14, 2024
12 checks passed
@chrisc36
Copy link
Contributor Author

chrisc36 commented Aug 14, 2024

Yea that description is definitely confusing, it makes it sound like True causes it to not save the state, but I assume that was not intentional since that contradicts the note and the parameter name.

@dirkgr
Copy link
Member

dirkgr commented Aug 15, 2024

Can't we just set this to True?

The reason this is there in the first place is that it used to be broken when combined with torch.compile(). So it wasn't a perf thing, it was just that a part was broken that we didn't need anyways, so I wanted to disable it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants