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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increment port when taken #2140

Closed
wants to merge 1 commit into from
Closed

Conversation

ZhaofengWu
Copy link
Contributor

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #2081. When a port is already taken and if we are under a pre-set retry limit, increment the port number and try again. In fact interestingly this missing functionality is currently mentioned in an in-line comment.
https://github.com/PyTorchLightning/pytorch-lightning/blob/bd49b07fbba09b1e7d8851ee5a1ffce3d5925e9e/pytorch_lightning/trainer/distrib_data_parallel.py#L426

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 馃檭

@mergify mergify bot requested a review from a team June 10, 2020 20:22
@Borda Borda added the feature Is an improvement or enhancement label Jun 10, 2020
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

Merging #2140 into master will decrease coverage by 0%.
The diff coverage is 0%.

@@          Coverage Diff           @@
##           master   #2140   +/-   ##
======================================
- Coverage      87%     87%   -0%     
======================================
  Files          68      68           
  Lines        5180    5188    +8     
======================================
  Hits         4508    4508           
- Misses        672     680    +8     

@williamFalcon
Copy link
Contributor

ummm... we need to test this a bit more. i think it was there, then it was removed because it didn't work well. let's hash this out for 0.9.0

@williamFalcon williamFalcon added this to the 0.9.0 milestone Jun 13, 2020
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2020

This pull request is now in conflict... :(

@williamFalcon
Copy link
Contributor

@ZhaofengWu
Ok, i tried this way but it doesn't actually work. I'll add you as co-author to #2231 which does work (just tried it). I use a slightly different approach there.

@Borda
Copy link
Member

Borda commented Jun 18, 2020

@williamFalcon GH does not support anything like the co-author of a PR
the only way how to solve this situation is merge his PR to yours and keep his PR id in the changelog as I did here for example #1920 (comment)
well we mined co-authors once or twice to be listed as release contributors but it was too time demanding as I had to do it manually and manually pair mails and GH users so now we just list PR authors from the changelog
so I am adding this PR to the changelog next to yours, see #2181

@Borda Borda modified the milestones: 0.9.0, 0.8.0 Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeError: Address already in use on 'ddp' mode pl 0.8.0
3 participants