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

Replace .step(synchronize=False) with optimizer.skip_synchronize() #1132

Merged
merged 3 commits into from Jun 11, 2019

Conversation

3 participants
@alsrgv
Copy link
Collaborator

commented Jun 10, 2019

NVIDIA AMP does not support passing additional flags to optimizer.step(), such as optimizer.step(synchronize=False).

This PR switches API to use context manager:

optimizer.synchronize()
with optimizer.skip_synchronize():
    optimizer.step()

@alsrgv alsrgv requested review from tgaddair and abditag2 Jun 10, 2019

@alsrgv alsrgv self-assigned this Jun 10, 2019

@alsrgv alsrgv force-pushed the fix_amp_support branch 2 times, most recently from 1d3f0e5 to 6a0091e Jun 10, 2019

Replace .step(synchronize=False) with optimizer.already_synchronized()
Signed-off-by: Alex Sergeev <alsrgv@users.noreply.github.com>

@alsrgv alsrgv force-pushed the fix_amp_support branch from 6a0091e to d72ad23 Jun 10, 2019

@abditag2

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2019

New interface LGTM. Shall we keep the old interface for backward compatibility and raise a warning and remove it in a later release?

A context manager used to specify that optimizer.step() should
not perform synchronization.
It's typically used in a following pattern:

This comment has been minimized.

Copy link
@abditag2

abditag2 Jun 10, 2019

Collaborator

nit: It's typically used in a following pattern: >> It's typically used in the following pattern:

Fix docs
Signed-off-by: Alex Sergeev <alsrgv@users.noreply.github.com>
@alsrgv

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 10, 2019

@abditag2, I'm debating that, since the API was released a couple of days ago and it does not work for the only user who asked for it. Seems less confusing to just hard-migrate to a new API at this point.

@abditag2

This comment has been minimized.

Copy link
Collaborator

commented Jun 10, 2019

Sounds good then!

Rename to skip_synchronize() and fix test
Signed-off-by: Alex Sergeev <alsrgv@users.noreply.github.com>

@alsrgv alsrgv changed the title Replace .step(synchronize=False) with optimizer.already_synchronized() Replace .step(synchronize=False) with optimizer.skip_synchronize() Jun 10, 2019

@alsrgv alsrgv merged commit 049f108 into master Jun 11, 2019

3 checks passed

DCO DCO
Details
License Compliance All checks passed.
Details
buildkite/horovod/pr Build #474 passed (44 minutes, 7 seconds)
Details

@alsrgv alsrgv deleted the fix_amp_support branch Jun 11, 2019

@alsrgv alsrgv referenced this pull request Jun 11, 2019

Merged

Bump version to 0.16.4 #1139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.