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

Disk #706

Closed
wants to merge 10 commits into from
Closed

Disk #706

wants to merge 10 commits into from

Conversation

564612540
Copy link
Contributor

@564612540 564612540 commented Dec 23, 2024

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

It introduces a set of new optimizers called DiSK, which uses a simplified Kalman filter to improve optimizer performance.

How Has This Been Tested (if it applies)

It is tested with the mnist.py from the example folder (with modifications for DiSK) to ensure all the functions work.

Checklist

Not sure whether to add documents.

  • 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.

@facebook-github-bot
Copy link
Contributor

Hi @564612540!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@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 Dec 23, 2024
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. (Because this pull request was imported automatically, there will not be any future comments.)

Copy link
Contributor

@iden-kalemaj iden-kalemaj left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! Looks good to me.

Copy link
Contributor

@iden-kalemaj iden-kalemaj left a comment

Choose a reason for hiding this comment

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

Could you please fix the linting errors here: https://github.com/pytorch/opacus/actions/runs/12486817231/job/35330455867?pr=706.

For more instructions on linting please see the instructions here under "Code style": https://github.com/pytorch/opacus/blob/53b3c25432bb75dd92b22131d02fcdc39c8dbe5f/CONTRIBUTING.md.

Hopefully, the changes should be minor. Please let us know if there are any questions. Following a unified code style helps us maintain code readability.

@564612540
Copy link
Contributor Author

The error is fixed

@iden-kalemaj
Copy link
Contributor

iden-kalemaj commented Jan 10, 2025

It seems that there is still an issue with isort. Could you try
isort -l 88 --lines-after-imports 2 -m 3 --trailing-comma KFprivacy_engine.py

If your isort version is <5.0.0 please use
isort -l 88 --lines-after-imports 2 -m 3 --trailing-comma --recursive from the disk_optimizer directory to fix all files recursively.

Apologies for the back and forth.

@564612540
Copy link
Contributor Author

isort -l 88 --lines-after-imports 2 -m 3 --trailing-comma KFprivacy_engine.py

It is not showing any error from my side when running this command. I don't know what is the issue here.

The logs are:

PS D:\Dropbox\Research\Projects\Experiments\Python\opacus\research\disk_optimizer> isort -l 88 --lines-after-imports 2 -m 3 --trailing-comma .\KFprivacy_engine.py -v

                 _                 _
                (_) ___  ___  _ __| |_
                | |/ _/ / _ \/ '__  _/
                | |\__ \/\_\/| |  | |_
                |_|\___/\___/\_/   \_/

      isort your imports, so you don't have to.

                    VERSION 5.11.5

from-type place_module for typing returned STDLIB
from-type place_module for opacus.optimizers returned THIRDPARTY
from-type place_module for opacus.privacy_engine returned THIRDPARTY
from-type place_module for torch returned THIRDPARTY
from-type place_module for optimizers returned FIRSTPARTY

@iden-kalemaj
Copy link
Contributor

iden-kalemaj commented Jan 14, 2025

That is strange. For now, can you please use the following import order in KFprivacy_engine.py:

from typing import List, Union

from opacus.optimizers import DPOptimizer
from opacus.privacy_engine import PrivacyEngine
from optimizers import KF_DPOptimizer, get_optimizer_class
from torch import optim


class KF_PrivacyEngine(PrivacyEngine):

and re-run black and isort.

@564612540
Copy link
Contributor Author

That is strange. For now, can you please use the following import order in KFprivacy_engine.py:

from typing import List, Union

from opacus.optimizers import DPOptimizer
from opacus.privacy_engine import PrivacyEngine
from optimizers import KF_DPOptimizer, get_optimizer_class
from torch import optim


class KF_PrivacyEngine(PrivacyEngine):

and re-run black and isort.

isort automatically changes it back to the previous format below:

from typing import List, Union

from opacus.optimizers import DPOptimizer
from opacus.privacy_engine import PrivacyEngine
from torch import optim

from optimizers import KF_DPOptimizer, get_optimizer_class


class KF_PrivacyEngine(PrivacyEngine):

Now I am changing to

from typing import List, Union

from opacus.optimizers import DPOptimizer
from opacus.privacy_engine import PrivacyEngine
from torch import optim

from .optimizers import KF_DPOptimizer, get_optimizer_class

I think this should work

@facebook-github-bot
Copy link
Contributor

@iden-kalemaj merged this pull request in b4c075d.

@iden-kalemaj
Copy link
Contributor

iden-kalemaj commented Jan 17, 2025

Thank you again for your contribution.

We would like to suggest adding copy-right headers to each of the .py files you contributed. The copy-right header would look something like this for an individual contributor

# Copyright (c) FirstName LastName

or

# Copyright (c) CompanyName

if contributing from a company.

This is for consistency with files contributed from Meta all of which have copyright headers.

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. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants