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

Warn and ignore "drop_last" when set in DPDataLoader #357

Closed
wants to merge 1 commit into from

Conversation

gautham-kollu
Copy link

Summary: Add a warning to show "drop_last" is being ignored and is not compatible with DPDataLoader

Differential Revision: D34197820

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Feb 14, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D34197820

Copy link
Contributor

@karthikprasad karthikprasad left a comment

Choose a reason for hiding this comment

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

Hi @gautham-kollu , thanks for taking this up. I just added a couple of nits, looks good otherwise. Happy to approve after the changes :)

@@ -143,13 +144,15 @@ def __init__(
if collate_fn is None:
collate_fn = default_collate

if drop_last:
warnings.warn("Ignoring drop_last as it is not compatible with DPDataLoader.")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: As per @romovpa's comment on #352, warning is probably not necessary as the client doesn't have to take any action. How about changing this to logger.warning() instead?


dataset = TensorDataset(x)
data_loader = DataLoader(dataset, drop_last=True)
dp_data_loader = DPDataLoader.from_data_loader(data_loader)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pacify linter

Suggested change
dp_data_loader = DPDataLoader.from_data_loader(data_loader)
_ = DPDataLoader.from_data_loader(data_loader)

gautham-kollu added a commit to gautham-kollu/opacus that referenced this pull request Feb 14, 2022
Summary:
Pull Request resolved: pytorch#357

Add a warning to show "drop_last" is being ignored and is not compatible with DPDataLoader

Differential Revision: D34197820

fbshipit-source-id: 6e7d1ba69c6018a4219bc9e8640c61fac806f8cf
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D34197820

Summary:
Pull Request resolved: pytorch#357

Add a warning to show "drop_last" is being ignored and is not compatible with DPDataLoader

Differential Revision: D34197820

fbshipit-source-id: 1ff0b0ac4a0b4b6248e6b95e532a928e0ef0be02
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D34197820

Copy link
Contributor

@karthikprasad karthikprasad left a comment

Choose a reason for hiding this comment

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

Thanks again!

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

Successfully merging this pull request may close these issues.

In opacus.data_loader, batch_sampler option is mutually exclusive with drop_last
3 participants