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

Fixed PYTHONPATH for ddp test model #4528

Merged

Conversation

gianscarpe
Copy link
Contributor

@gianscarpe gianscarpe commented Nov 5, 2020

What does this PR do?

I found an issue related to PYTHONPATH env when launching test_ddp.py. In particular, the pytorch-lightning local directory should be placed before the installation directory to access tests directory locally. This solved the issue and tests now pass

Fixes #3827

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes? No need
  • Did you write any new necessary tests? No need
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

  • Is this pull request ready for review? (if not, please submit in draft mode)

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?

Yes! I want to continue improving pytorch-lightning and cooperate!🙃

@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #4528 (ee47060) into master (4bb3a08) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4528   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         129     129           
  Lines        9359    9359           
======================================
  Hits         8677    8677           
  Misses        682     682           

@@ -29,11 +29,10 @@ def call_training_script(module_file, cli_args, method, tmpdir, timeout=60):

# need to set the PYTHONPATH in case pytorch_lightning was not installed into the environment
env = os.environ.copy()
env['PYTHONPATH'] = f'{pytorch_lightning.__file__}:' + env.get('PYTHONPATH', '')
env['PYTHONPATH'] = env.get('PYTHONPATH', '') + f'{pytorch_lightning.__file__}:'
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why the self-file shall not be the first one... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my setup, I installed pytorch-lightning and its dependencies. When test_model is launched as subprocess, it uses tests module from the virtualenv instead of the local 'tests' as supposed (and PYTHONPATH contains local pytorch-lightning first)

@Borda Borda added bug Something isn't working discussion In a discussion stage labels Nov 5, 2020
@Borda Borda added this to the 1.0.x milestone Nov 5, 2020
@edenlightning edenlightning modified the milestones: 1.0.x, 1.0.7 Nov 10, 2020
@Borda Borda modified the milestones: 1.0.7, 1.0.x Nov 11, 2020
@edenlightning edenlightning modified the milestones: 1.0.x, 1.0.7 Nov 13, 2020
@Borda Borda modified the milestones: 1.0.7, 1.0.x Nov 13, 2020
@gianscarpe gianscarpe requested a review from Borda November 16, 2020 15:14
@Borda
Copy link
Member

Borda commented Nov 16, 2020

@tchaton @awaelchli mind check on this DDP?

@Borda Borda added the distributed Generic distributed-related topic label Nov 16, 2020
@Borda Borda modified the milestones: 1.0.x, 1.1 Nov 18, 2020
@Borda
Copy link
Member

Borda commented Nov 30, 2020

@gianscarpe mind resolve conflicts?
@awaelchli @tchaton mind verify the fix... 🐰

@awaelchli
Copy link
Contributor

Yes, verified, this should be fine :)

@awaelchli awaelchli added the ready PRs ready to be merged label Dec 3, 2020
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

lgtm

@SeanNaren SeanNaren merged commit 16fa4ed into Lightning-AI:master Dec 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discussion In a discussion stage distributed Generic distributed-related topic ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate EvalModelTemplatein favor of BoringModel and another simple model that does actually learn
6 participants