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

require python 3.7 + minor refactoring #277

Merged
merged 4 commits into from
Nov 25, 2021
Merged

Conversation

ffuuugor
Copy link
Contributor

Following some internal test failures on #273, we've decided to up min required python version (we use annotations from future, which are only introduced in 3.7). Since 3.7 is already a default in colab, and is pretty popular overall, I don't see many potential issues with it.

One more change (which probably should have been decoupled into a separate pull request) is accountant hook refactoring. While writing guide on advanced usage of opacus, I realised that accountant hook is always the same for a given accountant. It can be different for different accountants, but same mechanism always requires the same hook.

This change makes it easier to attach accountant outside of PrivacyEngine

@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 Nov 24, 2021
@ffuuugor
Copy link
Contributor Author

From offline discussion with @karthikprasad :

a) All things equal we don't want to add unnecessary restrictions. We want to only set requirements that are based on which features we're using
b) We should only support versions we can regularly test. if we can't set up regular testing with a certain python version, it'll be too easy to slip and introduce breaking change

With that, we're choosing the oldest python 3.7 version available on circleCI: 3.7.4

@ffuuugor ffuuugor merged commit 1357baf into experimental_v1.0 Nov 25, 2021
@karthikprasad karthikprasad deleted the ffuuugor_ver branch November 25, 2021 22:46
karthikprasad added a commit to karthikprasad/opacus that referenced this pull request Nov 30, 2021
Summary:
This is an update to pytorch#277 to keep up with the changes of fb97c38.

Also fix a link in Migration Guide along the way.

Differential Revision: D32715608

fbshipit-source-id: c238a104ddbe74e2399888e680f378f764bd5ec8
karthikprasad added a commit to karthikprasad/opacus that referenced this pull request Nov 30, 2021
Summary:
Pull Request resolved: pytorch#293

This is an update to pytorch#277 to keep up with the changes of fb97c38.

Also fix a link in Migration Guide along the way.

Differential Revision: D32715608

fbshipit-source-id: ff3b9ed72e2254d9451f1623814cb11e4b331839
karthikprasad added a commit to karthikprasad/opacus that referenced this pull request Nov 30, 2021
Summary:
Pull Request resolved: pytorch#293

This is an update to pytorch#277 to keep up with the changes of fb97c38.

Also fix a link in Migration Guide along the way.

Differential Revision: D32715608

fbshipit-source-id: 6efc474eb9bbb424ad65a475d8c1484a384f0fd2
facebook-github-bot pushed a commit that referenced this pull request Nov 30, 2021
Summary:
Pull Request resolved: #293

This is an update to #277 to keep up with the changes of fb97c38.

Also fix a link in Migration Guide along the way.

Reviewed By: ffuuugor

Differential Revision: D32715608

fbshipit-source-id: a192b75636a42dcaf23acd0f25fa8f4955f2ec55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants