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 random seed for RA sampler #5053

Merged
merged 3 commits into from
Dec 8, 2021
Merged

support random seed for RA sampler #5053

merged 3 commits into from
Dec 8, 2021

Conversation

xiaohu2015
Copy link
Contributor

@xiaohu2015 xiaohu2015 commented Dec 8, 2021 •
edited by pytorch-probot bot
Loading

This pr is about #5051
Two modification:

  • support random seed for RA sampler
  • fix import bug for RA sampler

cc @datumbox

@facebook-github-bot
Copy link

facebook-github-bot commented Dec 8, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit 17ac681 (more details on the Dr. CI page):

  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See GitHub Actions build CodeQL / build (1/1)

Step: "Unknown" (full log | diagnosis details | 🔁 rerun)

2021-12-08T22:28:40.7675183Z ##[error]The operation was canceled. 
2021-12-08T22:28:37.4264768Z ##[group]Setup Python dependencies
2021-12-08T22:28:37.4267002Z [command]/home/runner/work/_actions/github/codeql-action/v1/python-setup/install_tools.sh
2021-12-08T22:28:37.4287000Z + set -e
2021-12-08T22:28:37.4289760Z + export PATH=/home/runner/.local/bin:/home/linuxbrew/.linuxbrew/bin:/home/linuxbrew/.linuxbrew/sbin:/home/runner/.local/bin:/opt/pipx_bin:/home/runner/.cargo/bin:/home/runner/.config/composer/vendor/bin:/usr/local/.ghcup/bin:/home/runner/.dotnet/tools:/snap/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin
2021-12-08T22:28:37.4293218Z + python3 -m pip install --user --upgrade pip setuptools wheel
2021-12-08T22:28:39.1557037Z Collecting pip
2021-12-08T22:28:39.1933827Z   Downloading pip-21.3.1-py3-none-any.whl (1.7 MB)
2021-12-08T22:28:40.1968918Z Collecting setuptools
2021-12-08T22:28:40.2064930Z   Downloading setuptools-59.5.0-py3-none-any.whl (952 kB)
2021-12-08T22:28:40.7047451Z ##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
2021-12-08T22:28:40.7675183Z ##[error]The operation was canceled.
2021-12-08T22:28:40.7771704Z Cleaning up orphan processes
This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

datumbox
datumbox reviewed Dec 8, 2021
View reviewed changes
references/classification/train.py
@@ -9,7 +9,7 @@
import torchvision
import transforms
import utils
from references.classification.sampler import RASampler
from sampler import RASampler
Copy link
Contributor

@datumbox datumbox Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch, if this PR is not merged could you please bring this separately?

references/classification/sampler.py
@@ -15,7 +15,7 @@ class RASampler(torch.utils.data.Sampler):
https://github.com/facebookresearch/deit/blob/main/samplers.py
"""

def __init__(self, dataset, num_replicas=None, rank=None, shuffle=True):
def __init__(self, dataset, num_replicas=None, rank=None, shuffle=True, seed=0):
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree this might be useful but it's also a paradigm we don't do on TorchVision. Don't get me wrong, this approach is popular with other libraries it's just that setting seeds on constructors is something we don't typically do.

@fmassa thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@datumbox The RA sampler is modified from torch.utils.data.DistributedSampler which also has a parameter seed. The default value of seed is 0, so it does not affect the behavior of RA samper if users don't explicitly set a seed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Since DistributedSampler supports it we can also have it.

@datumbox datumbox mentioned this pull request Dec 8, 2021
Closed
datumbox
datumbox approved these changes Dec 8, 2021
View reviewed changes
references/classification/sampler.py
@@ -15,7 +15,7 @@ class RASampler(torch.utils.data.Sampler):
https://github.com/facebookresearch/deit/blob/main/samplers.py
"""

def __init__(self, dataset, num_replicas=None, rank=None, shuffle=True):
def __init__(self, dataset, num_replicas=None, rank=None, shuffle=True, seed=0):
Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Since DistributedSampler supports it we can also have it.

@datumbox datumbox merged commit 4cacf5a into pytorch:main Dec 8, 2021
facebook-github-bot pushed a commit that referenced this pull request Dec 17, 2021
@NicolasHug @datumbox @facebook-github-bot
[fbsync] support random seed for RA sampler (#5053)
4e21c20 
Summary:
* support random seed used to shuffle the sampler

* fix bug for RA samper

Reviewed By: fmassa

Differential Revision: D33185002

fbshipit-source-id: ed0461edabbeb495923e76d7d5ff59a8e85f422e

Co-authored-by: Vasilis Vryniotis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datumbox datumbox datumbox approved these changes

Assignees
No one assigned
Labels
bug ciflow/default cla signed module: reference scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@xiaohu2015 @facebook-github-bot @datumbox