[RFC] Future structure #13265

Borda · 2022-06-10T11:46:04Z

What does this PR do?

it is just PoC, and it follows the latest PyPA project structure - https://github.com/pypa/sampleproject
it is said that using src is better; it does not accidentally use modules that are not installed...

so with this new structure, we would have

examples/  # the actual pl_examples
src/
 L pytorch_lightning  # the actual package in root
test/
 |- legacy  # downloaded tests data from S3
 L  unittests  # actual folder with tests

NOTE:

there is no change in user experience while installing this repo from the source
there shall not be any breaking changes for ongoing/open PRs; if GitHub flags some collisions, they shall be quickly resolved in the command line with git rebase master as it uses here way resolver
this will be merged as rebase & merge for traceability and simple resolving potential conflicts

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
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?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

cc @carmocca @akihironitta @Borda @tchaton @justusschock @awaelchli

codecov · 2022-06-10T11:55:51Z

Codecov Report

Merging #13265 (f3f852c) into master (5e4b712) will decrease coverage by 1%.
The diff coverage is 85%.

@@           Coverage Diff            @@
##           master   #13265    +/-   ##
========================================
- Coverage      89%      87%    -1%     
========================================
  Files         217      221     +4     
  Lines       18322    19014   +692     
========================================
+ Hits        16282    16620   +338     
- Misses       2040     2394   +354

.azure-pipelines/gpu-benchmark.yml

.gitignore

carmocca · 2022-06-14T17:37:14Z

.azure-pipelines/gpu-benchmark.yml

@@ -11,6 +11,7 @@ trigger:
    include:
      - "master"
      - "release/*"
+      - "future/*"


This should be removed before merge, right?

carmocca · 2022-06-14T17:54:54Z

.azure-pipelines/gpu-benchmark.yml

+    - bash: |
+        pip install -e . -r requirements/strategies.txt
+        pip list
+      displayName: 'Install package'


This is not necessary

carmocca · 2022-06-14T17:59:54Z

.azure-pipelines/gpu-tests.yml

@@ -86,8 +90,9 @@ jobs:
        python -m coverage report
        python -m coverage xml
        python -m coverage html
-        python -m codecov --token=$(CODECOV_TOKEN) --commit=$(Build.SourceVersion) --flags=gpu,pytest --name="GPU-coverage" --env=linux,azure
+        python -m codecov --token=$(CODECOV_TOKEN) --commit=$(Build.SourceVersion) --flags=gpu,unittest --name="GPU-coverage" --env=linux,azure


Suggested change

python -m codecov --token=$(CODECOV_TOKEN) --commit=$(Build.SourceVersion) --flags=gpu,unittest --name="GPU-coverage" --env=linux,azure

python -m codecov --token=$(CODECOV_TOKEN) --commit=$(Build.SourceVersion) --flags=gpu,pytest --name="GPU-coverage" --env=linux,azure

carmocca · 2022-06-14T18:05:56Z

.azure-pipelines/gpu-tests.yml

-        bash pl_examples/run_examples.sh --trainer.accelerator=gpu --trainer.devices=1
-        bash pl_examples/run_examples.sh --trainer.accelerator=gpu --trainer.devices=2 --trainer.strategy=ddp
-        bash pl_examples/run_examples.sh --trainer.accelerator=gpu --trainer.devices=2 --trainer.strategy=ddp --trainer.precision=16
+        bash run_ddp_examples.sh


Let's keep the test here with the other standalone tests

carmocca · 2022-06-14T18:31:48Z

.azure-pipelines/gpu-tests.yml

-        bash pl_examples/run_examples.sh --trainer.accelerator=gpu --trainer.devices=2 --trainer.strategy=ddp
-        bash pl_examples/run_examples.sh --trainer.accelerator=gpu --trainer.devices=2 --trainer.strategy=ddp --trainer.precision=16
+        bash run_ddp_examples.sh
+        bash run_pl_examples.sh --trainer.accelerator=gpu --trainer.devices=1


This script should be examples/pytorch/run_examples.sh

carmocca · 2022-06-14T18:33:22Z

.azure-pipelines/gpu-tests.yml

+        pip install -e .
+        pip install --requirement requirements/devel.txt
+        pip install --requirement requirements/strategies.txt


Suggested change

pip install -e .

pip install --requirement requirements/devel.txt

pip install --requirement requirements/strategies.txt

pip install -e .[strategies]

pip install --requirement requirements/devel.txt

carmocca · 2022-06-14T18:35:55Z

.azure-pipelines/hpu-tests.yml

      displayName: 'HPU precision test'

    - bash: |
        export PYTHONPATH="${PYTHONPATH}:$(pwd)"
-        python "pl_examples/hpu_examples/simple_mnist/mnist.py"
+        python "pl_hpu/mnist_sample.py"


The structure here should be examples/pytorch/hpu

carmocca · 2022-06-14T18:37:26Z

.azure-pipelines/ipu-tests.yml

-        python -m coverage run --source pytorch_lightning -m pytest tests -vv --junitxml=$(Build.StagingDirectory)/test-results.xml --durations=50
+        cd src
+        python -m pytest pytorch_lightning
+      displayName: 'DocTests'


I wouldn't add doctesting to these accelerator jobs as it complicates the pipeline for contributors

carmocca · 2022-06-14T18:38:18Z

.github/CODEOWNERS

-/docs/source/index.rst                  @williamfalcon
-/docs/source/levels                     @williamfalcon @RobertLaurella
-/docs/source/expertise_levels           @williamfalcon @RobertLaurella
+/docs/source-PL/conf.py                    @borda @awaelchli @carmocca


Why do we need this - separator instead of

docs/pytorch_lightning/source/...

carmocca · 2022-07-19T17:20:10Z

@Borda is there anything left to do here? Can we close this?

Borda · 2022-07-19T18:14:07Z

@Borda is there anything left to do here? Can we close this?

shall be fine, but would like to check it, run some diff that it is minimal... 🐰

stale · 2022-08-12T00:50:57Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

Borda added ci Continuous Integration let's do it! approved to implement design Includes a design discussion code quality labels Jun 10, 2022

Borda changed the title ~~Future structure~~ [RFC] Future structure Jun 10, 2022

Borda marked this pull request as ready for review June 10, 2022 15:46

Borda requested review from williamFalcon, carmocca, edenlightning, tchaton, awaelchli, ananthsub, SeanNaren, SkafteNicki, justusschock, kaushikb11, akihironitta and rohitgr7 as code owners June 10, 2022 15:46

Borda added this to the 1.7 milestone Jun 10, 2022

akihironitta reviewed Jun 14, 2022

View reviewed changes

.azure-pipelines/gpu-benchmark.yml Outdated Show resolved Hide resolved

.gitignore Show resolved Hide resolved

mergify bot added the has conflicts label Jun 14, 2022

Borda added 8 commits June 14, 2022 19:26

move: pytorch_lightning >> src/

f2c5287

move: pl_examples >> src/

4a11e74

move: legacy >> test/

41e2dc1

move: tests >> test/

701f5b6

rename unittests

bf07902

ci: trigger future

b01c18a

update setup & install

c4d737c

GHA: update paths

5fa7f0e

Borda added 12 commits June 14, 2022 19:27

CI: update Azure

276f011

docs: rename source >> source-PL

14bb3be

docs: fix typing

e02daef

CI: update CircleCI

35c2275

fix dockers

94f4bdf

rename unittests_pl

68028db

move MNIST to PL.demos

20cab40

convert pl_examples package to plain examples

ebcf029

update CI for examples

180e330

unify torch.Tensor >> Tensor

64090ec

readthedocs

bc973d6

update paths & codeowners

f3f852c

Borda force-pushed the future/structure branch from 79eceaa to f3f852c Compare June 14, 2022 17:27

mergify bot removed the has conflicts label Jun 14, 2022

carmocca reviewed Jun 14, 2022

View reviewed changes

carmocca mentioned this pull request Jun 14, 2022

Standardize tensor annotation #13292

Merged

9 tasks

This was referenced Jun 14, 2022

Future 1/n: package in src/ folder #13293

Merged

Future 2/n: stand-alone examples #13294

Merged

mergify bot added the has conflicts label Jun 14, 2022

This was referenced Jun 14, 2022

Future 4/n: test & legacy in test/ folder #13295

Merged

Future 3/n: docs adjustment #13299

Merged

Borda marked this pull request as draft June 17, 2022 01:33

carmocca removed this from the pl:1.7 milestone Jul 19, 2022

stale bot added the won't fix This will not be worked on label Aug 12, 2022

carmocca closed this Aug 26, 2022

carmocca deleted the future/structure branch August 26, 2022 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Future structure #13265

[RFC] Future structure #13265

Borda commented Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca Jun 14, 2022

carmocca commented Jul 19, 2022

Borda commented Jul 19, 2022 •

edited

Loading

stale bot commented Aug 12, 2022

	python -m codecov --token=$(CODECOV_TOKEN) --commit=$(Build.SourceVersion) --flags=gpu,unittest --name="GPU-coverage" --env=linux,azure
	python -m codecov --token=$(CODECOV_TOKEN) --commit=$(Build.SourceVersion) --flags=gpu,pytest --name="GPU-coverage" --env=linux,azure

[RFC] Future structure #13265

[RFC] Future structure #13265

Conversation

Borda commented Jun 10, 2022 • edited Loading

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

codecov bot commented Jun 10, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmocca commented Jul 19, 2022

Borda commented Jul 19, 2022 • edited Loading

stale bot commented Aug 12, 2022

Borda commented Jun 10, 2022 •

edited

Loading

codecov bot commented Jun 10, 2022 •

edited

Loading

Borda commented Jul 19, 2022 •

edited

Loading