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

[WIP] ref: part 6 of #3733 #3783

Closed
wants to merge 9 commits into from
Closed

[WIP] ref: part 6 of #3733 #3783

wants to merge 9 commits into from

Conversation

williamFalcon
Copy link
Contributor

ref: part 6 of #3733

@mergify mergify bot requested a review from a team October 2, 2020 01:51
if is_master:
available_gpus = os.environ['CUDA_VISIBLE_DEVICES'].split(',')
gpu_idx = int(available_gpus[self.trainer.local_rank])
gpu_idx = int(os.environ.get('PL_DDP_PID', process_idx))
Copy link
Contributor

Choose a reason for hiding this comment

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

is_master is never used here. it seems like ddp_train doesn't need the self.mode == 'ddp' check anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!
might be a weird merge artifact... let me check.

Comment on lines +147 to +148
if 'WORLD_SIZE' in os.environ:
del os.environ['WORLD_SIZE']
Copy link
Contributor

Choose a reason for hiding this comment

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

n00b question: should this happen in a teardown step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably haha.

i'm not sure what will happen if we move it though. I want to lock down the tests then we can see where it goes

@Borda Borda added the refactor label Oct 2, 2020
@pep8speaks
Copy link

pep8speaks commented Oct 2, 2020

Hello @williamFalcon! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-02 11:15:22 UTC

@williamFalcon williamFalcon changed the title ref: part 6 of #3733 [WIP] ref: part 6 of #3733 Oct 2, 2020
@williamFalcon williamFalcon marked this pull request as draft October 2, 2020 10:56
@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #3783 into master will decrease coverage by 42%.
The diff coverage is 23%.

@@           Coverage Diff            @@
##           master   #3783     +/-   ##
========================================
- Coverage      85%     43%    -42%     
========================================
  Files         110     110             
  Lines        8537    8582     +45     
========================================
- Hits         7293    3708   -3585     
- Misses       1244    4874   +3630     

@edenlightning edenlightning modified the milestones: 0.9.x, 1.0 Oct 4, 2020
@Borda Borda deleted the fw2_6 branch October 5, 2020 20:37
@Borda Borda modified the milestones: 1.0, 0.10.0 Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants