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.

Sign up for GitHub

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

Jump to bottom

Support checkpoints #429

Closed
wants to merge 28 commits into from
Closed

Support checkpoints #429

wants to merge 28 commits into from

Conversation

karthikprasad
Copy link
Contributor

@karthikprasad karthikprasad commented May 17, 2022 •
edited
Loading

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Docs change / refactoring / dependency upgrade

Motivation and Context / Related issue

#373

How Has This Been Tested (if it applies)

Unit tests.

Checklist

  • The documentation is up-to-date with the changes I made.
  • I have read the CONTRIBUTING document and completed the CLA (see CONTRIBUTING).
  • All tests passed, and additional code has been covered with new tests.

@karthikprasad karthikprasad added the enhancement New feature or request label May 17, 2022
@karthikprasad karthikprasad added this to the 1.2.0 milestone May 17, 2022
@karthikprasad karthikprasad self-assigned this May 17, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2022
@karthikprasad karthikprasad linked an issue May 17, 2022 that may be closed by this pull request
Support checkpoints #373
Closed
@facebook-github-bot
Copy link
Contributor

@karthikprasad has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

ffuuugor
ffuuugor reviewed May 19, 2022
View reviewed changes
Copy link
Contributor

@ffuuugor ffuuugor left a comment

Choose a reason for hiding this comment

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

Not surprising at all, but amazing stuff! Thanks for the clean implementation and test coverage!

Slight ask: can we also have a new test in privacy_engine_test for the combined method?

opacus/grad_sample/grad_sample_module.py Outdated Show resolved Hide resolved
opacus/privacy_engine.py Outdated Show resolved Hide resolved
opacus/tests/accountants_test.py Show resolved Hide resolved
@karthikprasad
Copy link
Contributor Author

Thanks Igor!

can we also have a new test in privacy_engine_test for the combined method?

Yes, that is still pending, and it the only reason this PR is still in draft mode for this long. I'll add it soon and open it up for review :)

@ffuuugor
Copy link
Contributor

Yes, that is still pending, and it the only reason this PR is still in draft mode for this long. I'll add it soon and open it up for review :)

Whoops, sorry I jumped the gun - the PR looked so nice and polished, calling to get reviewed

Kevin-Abd and others added 3 commits May 23, 2022 16:09
@Kevin-Abd @karthikprasad
Minor fixex in FAQ (pytorch#430)
f69819b 
Summary:
## Types of changes

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [x] Docs change / refactoring / dependency upgrade

## Motivation and Context / Related issue

Ambiguous information about `alphas` and minor bugs in FAQ

`privacy_engine` has no function `get_privacy_spent(delta)`, but it's `accountant` does.
Also, the call requires delta to be passed as a named argument.

There is no mention of `alphas` in the `privacy_engine` documentation, and I had to follow the code to figure it out.

## How Has This Been Tested (if it applies)
N/A

## Checklist

- [ ] The documentation is up-to-date with the changes I made.
*N/A, no code changed*
- [ ] I have read the **CONTRIBUTING** document and completed the CLA (see **CONTRIBUTING**).
*I could not find any topic of 'CLA' in the **CONTRIBUTING** document*
- [ ] All tests passed, and additional code has been covered with new tests.
*N/A, no code changed*

Pull Request resolved: pytorch#430

Reviewed By: ashkan-software

Differential Revision: D36531555

Pulled By: karthikprasad

fbshipit-source-id: 97ef2b309b657d0910769f10cf48fd5661004236
@karthikprasad
disable poisson sampling in checkpoints test
86a8e0d
@karthikprasad
rebase
d3591b0
@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@karthikprasad karthikprasad marked this pull request as ready for review May 23, 2022 23:15
@facebook-github-bot
Copy link
Contributor

@karthikprasad has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ffuuugor
ffuuugor reviewed May 24, 2022
View reviewed changes
docs/faq.md Outdated Show resolved Hide resolved
opacus/grad_sample/grad_sample_module.py Outdated Show resolved Hide resolved
opacus/grad_sample/grad_sample_module.py Outdated Show resolved Hide resolved
opacus/privacy_engine.py Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@karthikprasad karthikprasad requested a review from ffuuugor May 31, 2022 19:12
@karthikprasad karthikprasad mentioned this pull request May 31, 2022
Open
@facebook-github-bot
Copy link
Contributor

@karthikprasad has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@karthikprasad has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ffuuugor
ffuuugor approved these changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@ffuuugor ffuuugor left a comment

Choose a reason for hiding this comment

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

Thanks for the amazing diff and for addressing the comments!
Tests seem to broken in main too, approving

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffuuugor ffuuugor ffuuugor approved these changes

@alexandresablayrolles alexandresablayrolles Awaiting requested review from alexandresablayrolles

Assignees

@karthikprasad karthikprasad

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

Support checkpoints
4 participants
@karthikprasad @facebook-github-bot @ffuuugor @Kevin-Abd