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

resolving documentation warnings #833

Merged
merged 35 commits into from
Feb 27, 2020

Conversation

hanbyul-kim
Copy link
Contributor

@hanbyul-kim hanbyul-kim commented Feb 13, 2020

Before submitting

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

What does this PR do?

Fixes #717 (issue).
remove warnings (number: 142 -> 9)

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 🙃

@Borda
Copy link
Member

Borda commented Feb 13, 2020

@hanbyul-kim could you pls check the lint issues:

./pytorch_lightning/core/lightning.py:355: [E501] line too long (172 > 120 characters)
./pytorch_lightning/core/lightning.py:357: [E501] line too long (207 > 120 characters)
./pytorch_lightning/trainer/distrib_parts.py:278: [E501] line too long (152 > 120 characters)
./pl_examples/domain_templates/gan.py:25: [E302] expected 2 blank lines, found 1
./pl_examples/domain_templates/gan.py:196: [F821] undefined name 'pl'
./pl_examples/basic_examples/lightning_module_template.py:21: [E302] expected 2 blank lines, found 1

@Borda Borda requested a review from neggert February 13, 2020 21:48
@Borda Borda added docs Documentation related feature Is an improvement or enhancement good first issue Good for newcomers labels Feb 13, 2020
@Borda Borda added this to the 0.6.1 milestone Feb 13, 2020
@pep8speaks
Copy link

pep8speaks commented Feb 16, 2020

Hello @hanbyul-kim! Thanks for updating this PR.

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

Comment last updated at 2020-02-27 16:05:30 UTC

@hanbyul-kim
Copy link
Contributor Author

@hanbyul-kim could you pls check the lint issues:

./pytorch_lightning/core/lightning.py:355: [E501] line too long (172 > 120 characters)
./pytorch_lightning/core/lightning.py:357: [E501] line too long (207 > 120 characters)
./pytorch_lightning/trainer/distrib_parts.py:278: [E501] line too long (152 > 120 characters)
./pl_examples/domain_templates/gan.py:25: [E302] expected 2 blank lines, found 1
./pl_examples/domain_templates/gan.py:196: [F821] undefined name 'pl'
./pl_examples/basic_examples/lightning_module_template.py:21: [E302] expected 2 blank lines, found 1

Sure. I've been busy this weekend. I'll come back and do it tonight.

@hanbyul-kim
Copy link
Contributor Author

hanbyul-kim commented Feb 16, 2020

@Borda I can't build docs anymore locally. Do you have any idea about this? I tried make clean; make install.

Partial import of `torchlightning` during the build process.
WARNING: while setting up extension sphinxcontrib.mockautodoc: directive 'automodule' is already registered, it will be overridden

Sphinx error:
Builder name install not registered or available through entry point
make: *** [install] Error 2

@Borda
Copy link
Member

Borda commented Feb 16, 2020

@hanbyul-kim Could you please rebase on the master, I will check the build...

@Borda
Copy link
Member

Borda commented Feb 16, 2020

Well it works for me and also the CI pass docs build

done
copying static files... ... done
copying extra files... done
dumping search index in English (code: en)... done
dumping object inventory... done
build succeeded, 140 warnings.

The HTML pages are in build/html.

@hanbyul-kim
Copy link
Contributor Author

@Borda Hi good morning. I think I managed to pass all the tests. Please review my code and approve it. 😆

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 🚀

@Borda Borda changed the title remove warnings (number: 142 -> 9) #717 resolving documentation warnings Feb 17, 2020
@Borda Borda added the ready PRs ready to be merged label Feb 17, 2020
@hanbyul-kim
Copy link
Contributor Author

@neggert Hi, I made a change to resolve warning issues in docs with help of @Borda. Could you please review my changes?

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 🚀 @williamFalcon ?

docs/source/index.rst Show resolved Hide resolved
@Borda Borda added the priority: 0 High priority task label Feb 20, 2020
@Borda
Copy link
Member

Borda commented Feb 20, 2020

@williamFalcon this also fixes missing examples in actual docs
see: https://pytorch-lightning.readthedocs.io/en/latest/pl_examples.domain_templates.gan.html

@Borda
Copy link
Member

Borda commented Feb 20, 2020

moreover, this shall prevent future issues #844

@williamFalcon
Copy link
Contributor

================================================================================================= FAILURES =================================================================================================
_______________________________________________________________________________________________ FLAKE8-check _______________________________________________________________________________________________
/home/waf251/Developer/pytorch-lightning/pytorch_lightning/trainer/trainer.py:30: [F811] redefinition of unused 'LightningModule' from line 21

@williamFalcon
Copy link
Contributor

Failing GPU tests...
Mind fixing and/or rebasing master?

_____________________________________________________________________________________________ test_amp_gpu_ddp _____________________________________________________________________________________________

tmpdir = local('/tmp/pytest-of-waf251/pytest-6/test_amp_gpu_ddp0')

    def test_amp_gpu_ddp(tmpdir):
        """Make sure DDP + AMP work."""
        if not tutils.can_run_gpu_test():
            return

        tutils.reset_seed()
        tutils.set_random_master_port()

        hparams = tutils.get_hparams()
        model = LightningTestModel(hparams)

        trainer_options = dict(
            default_save_path=tmpdir,
            show_progress_bar=True,
            max_epochs=1,
            gpus=2,
            distributed_backend='ddp',
            precision=16
        )

>       tutils.run_model_test(trainer_options, model)

tests/test_amp.py:81:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
tests/models/utils.py:63: in run_model_test
    result = trainer.fit(model)
pytorch_lightning/trainer/trainer.py:904: in fit
    mp.spawn(self.ddp_train, nprocs=self.num_gpus, args=(model,))
../../media/falcon_kcgscratch1/software/miniconda3/envs/pl4/lib/python3.8/site-packages/torch/multiprocessing/spawn.py:162: in spawn
    process.start()
../../media/falcon_kcgscratch1/software/miniconda3/envs/pl4/lib/python3.8/multiprocessing/process.py:121: in start
    self._popen = self._Popen(self)
../../media/falcon_kcgscratch1/software/miniconda3/envs/pl4/lib/python3.8/multiprocessing/context.py:283: in _Popen
    return Popen(process_obj)
../../media/falcon_kcgscratch1/software/miniconda3/envs/pl4/lib/python3.8/multiprocessing/popen_spawn_posix.py:32: in __init__
    super().__init__(process_obj)
../../media/falcon_kcgscratch1/software/miniconda3/envs/pl4/lib/python3.8/multiprocessing/popen_fork.py:19: in __init__
    self._launch(process_obj)
../../media/falcon_kcgscratch1/software/miniconda3/envs/pl4/lib/python3.8/multiprocessing/popen_spawn_posix.py:47: in _launch
    reduction.dump(process_obj, fp)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

obj = <SpawnProcess name='SpawnProcess-3' parent=29993 initial>, file = <_io.BytesIO object at 0x7f962dc853b0>, protocol = None

    def dump(obj, file, protocol=None):
        '''Replacement for pickle.dump() using ForkingPickler.'''
>       ForkingPickler(file, protocol).dump(obj)
E       AttributeError: Can't pickle local object 'ModelCheckpoint.__init__.<locals>.<lambda>'

../../media/falcon_kcgscratch1/software/miniconda3/envs/pl4/lib/python3.8/multiprocessing/reduction.py:60: AttributeError

@williamFalcon williamFalcon added Failed GPU tests and removed ready PRs ready to be merged labels Feb 22, 2020
@hanbyul-kim
Copy link
Contributor Author

hanbyul-kim commented Feb 22, 2020

@williamFalcon I didn't touch that gpu part, so I'm not sure that should be resolved in this issue. I guess this issue has something to do with docs. How do you think about it?

@Borda Borda mentioned this pull request Feb 22, 2020
@Borda
Copy link
Member

Borda commented Feb 22, 2020

@hanbyul-kim it maybe was there before... pls check the solution here #849 (comment)

@hanbyul-kim
Copy link
Contributor Author

@Borda @williamFalcon Rebased master again and pushed. Please review it again.

@hanbyul-kim
Copy link
Contributor Author

hanbyul-kim commented Feb 24, 2020

I'm failing CI test on macos build job (python3.7 latest).. maybe related to #859 ?

@hanbyul-kim
Copy link
Contributor Author

hanbyul-kim commented Feb 24, 2020

@williamFalcon I don't have a ddp machine, so I can't run the gpu test. However, this branch has rebased and 2244f8c was applied. So I guess there won't be similar gpu test problems.

@Borda
Copy link
Member

Borda commented Feb 24, 2020

just tried to rerun checks manually... LGTM @williamFalcon can you run GPU to get it done?

@awaelchli awaelchli mentioned this pull request Feb 26, 2020
5 tasks
@Borda
Copy link
Member

Borda commented Feb 26, 2020

@williamFalcon this one starting to be a bit old... could make it done just after @hanbyul-kim rebase it?

tests/test_profiler.py Outdated Show resolved Hide resolved
tests/test_profiler.py Outdated Show resolved Hide resolved
tests/test_profiler.py Outdated Show resolved Hide resolved
@Borda Borda added the ready PRs ready to be merged label Feb 27, 2020
@williamFalcon williamFalcon merged commit 563e2ba into Lightning-AI:master Feb 27, 2020
@Borda Borda mentioned this pull request Feb 27, 2020
5 tasks
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Apr 3, 2020
* add more underline

* fix LightningMudule import error

* remove unneeded blank line

* escape asterisk to fix inline emphasis warning

* add PULL_REQUEST_TEMPLATE.md

* add __init__.py and import imagenet_example

* fix duplicate label

* add noindex option to fix duplicate object warnings

* remove unexpected indent

* refer explicit LightningModule

* fix minor bug

* refer EarlyStopping explicitly

* restore exclude patterns

* change the way how to refer class

* remove unused import

* update badges & drop Travis/Appveyor (Lightning-AI#826)

* drop Travis

* drop Appveyor

* update badges

* fix missing PyPI images & CI badges (Lightning-AI#853)

* docs - anchor links (Lightning-AI#848)

* docs - add links

* add desc.

* add Greeting action (Lightning-AI#843)

* add Greeting action

* Update greetings.yml

Co-authored-by: William Falcon <waf2107@columbia.edu>

* add pep8speaks (Lightning-AI#842)

* advanced profiler describe + cleaned up tests (Lightning-AI#837)

* add py36 compatibility

* add test case to capture previous bug

* clean up tests

* clean up tests

* Update lightning_module_template.py

* Update lightning.py

* respond lint issues

* break long line

* break more lines

* checkout conflicting files from master

* shorten url

* checkout from upstream/master

* remove trailing whitespaces

* remove unused import LightningModule

* fix sphinx bot warnings

* Apply suggestions from code review

just to trigger CI

* Update .github/workflows/greetings.yml

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: William Falcon <waf2107@columbia.edu>
Co-authored-by: Jeremy Jordan <13970565+jeremyjordan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related feature Is an improvement or enhancement good first issue Good for newcomers priority: 0 High priority task ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix docs formatting
5 participants