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

Use random default port instead of a fix one #587

Closed
wants to merge 1 commit into from
Closed

Use random default port instead of a fix one #587

wants to merge 1 commit into from

Conversation

magic282
Copy link

@magic282 magic282 commented Dec 5, 2019

Before submitting

  • [y] Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
  • [y] Did you read the contributor guideline?
  • [y] Did you make sure to update the docs?
  • [n] Did you write any new necessary tests?

What does this PR do?

Fixes #485.

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@@ -603,7 +603,8 @@ def init_ddp_connection(self):
default_port = int(default_port) + 15000

except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

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

pls, drop the as e

@@ -603,7 +603,8 @@ def init_ddp_connection(self):
default_port = int(default_port) + 15000

except Exception as e:
default_port = 12910
import random
default_port = random.randint(15000,65535)
Copy link
Member

Choose a reason for hiding this comment

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

user numpy.random.randint()

@williamFalcon
Copy link
Contributor

actually, this won’t work.
All processes need the same port. making the port random will break ddp

@Borda
Copy link
Member

Borda commented Dec 5, 2019

so what about random port which will be the save over the package?

@williamFalcon
Copy link
Contributor

how? ddp starts a process independently for each GPU, and you can’t guarantee the same number.

@Borda
Copy link
Member

Borda commented Dec 5, 2019

You would have to set it in init, but I need to have look at it in detail...

@williamFalcon
Copy link
Contributor

can’t set in init because the processes all run at different times without guarantee of the same start time

@magic282
Copy link
Author

magic282 commented Dec 5, 2019

How about hashing the python cmd args to a get a fix random number for all processes as a workaround?

@Borda
Copy link
Member

Borda commented Dec 5, 2019

well if you set numpy.random.seed(42)then all random numbers are the same, it is commonly used for tests with random matrixes to have every time/run the same random numbers...

@williamFalcon
Copy link
Contributor

williamFalcon commented Dec 5, 2019

random seed is standard for anything ML. that doesn’t solve the problem though. but hashing the command might be a good option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random Master Port Number
3 participants