Rewrite accelerator_connector #11448

four4fish · 2022-01-12T18:46:43Z

What does this PR do?

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 internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read 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 🙃

tchaton · 2022-01-20T07:50:14Z

Hey @four4fish,

Any ETA for this PR to be ready ?

four4fish · 2022-01-20T18:50:44Z

Hey @four4fish,

Any ETA for this PR to be ready ?

@tchaton I should be able to fix tests early next week, then it will be ready for review.
This PR could be breakdown to smaller piece after reviews

tests/strategies/test_ddp_strategy.py

tests/trainer/test_trainer.py

tests/accelerators/test_accelerator_connector.py

carmocca

This PR is large so I would suggest you comment on your own PR to kickstart discussions on open questions

pytorch_lightning/trainer/connectors/accelerator_connector.py

pytorch_lightning/utilities/exceptions.py

pytorch_lightning/trainer/trainer.py

pytorch_lightning/trainer/connectors/accelerator_connector.py

pytorch_lightning/strategies/ddp.py

pytorch_lightning/strategies/single_device.py

pytorch_lightning/trainer/connectors/accelerator_connector.py

Co-authored-by: Carlos Mocholí <[email protected]>

for more information, see https://pre-commit.ci

…h-lightning into rewrite/acc_con

carmocca

Funny how GitHub crashes when trying to review the accelerator connector file.

There's many things that should be improved here before the release. Approving to unblock but let's try to move quick on the follow-ups

Good job!

tests/accelerators/test_accelerator_connector.py

pytorch_lightning/trainer/trainer.py

tests/accelerators/test_accelerator_connector.py

tests/accelerators/test_ipu.py

tests/accelerators/test_tpu.py

tests/trainer/flags/test_env_vars.py

Co-authored-by: Carlos Mocholí <[email protected]>

tchaton · 2022-02-18T08:16:03Z

connector

I thought this was only me ahahha @four4fish great job there, very impressed.

four4fish requested review from awaelchli, Borda, carmocca, justusschock, kaushikb11, SeanNaren, tchaton and williamFalcon as code owners January 12, 2022 18:46

four4fish marked this pull request as draft January 12, 2022 18:46

four4fish force-pushed the rewrite/acc_con branch 2 times, most recently from 254fb85 to 4ee369d Compare January 12, 2022 18:51

four4fish mentioned this pull request Jan 12, 2022

Rewrite Accelerator_connector and follow up tasks #11449

Closed

10 tasks

four4fish mentioned this pull request Jan 24, 2022

Lazy initialize Strategy.parallel_devices #11572

Merged

12 tasks

four4fish changed the title ~~[NOT ready for review] Rewrite accelerator_connector~~ Rewrite accelerator_connector Jan 24, 2022

four4fish force-pushed the rewrite/acc_con branch 3 times, most recently from 12b8bb9 to a0f80a6 Compare January 29, 2022 05:42

carmocca mentioned this pull request Jan 31, 2022

Fix typing for Accelerator Connector #11385

Closed

12 tasks

four4fish force-pushed the rewrite/acc_con branch 2 times, most recently from d67ed66 to d964949 Compare February 1, 2022 00:02

four4fish commented Feb 1, 2022

View reviewed changes

tests/strategies/test_ddp_strategy.py Outdated Show resolved Hide resolved

tests/trainer/test_trainer.py Show resolved Hide resolved

tests/accelerators/test_accelerator_connector.py Outdated Show resolved Hide resolved

carmocca reviewed Feb 2, 2022

View reviewed changes

ananthsub reviewed Feb 2, 2022

View reviewed changes

four4fish force-pushed the rewrite/acc_con branch from c96ddda to 2134c3b Compare February 3, 2022 02:29

four4fish marked this pull request as ready for review February 3, 2022 03:11

four4fish requested a review from rohitgr7 as a code owner February 3, 2022 03:11

mergify bot added has conflicts and removed has conflicts labels Feb 3, 2022

four4fish and others added 4 commits February 17, 2022 10:50

Apply suggestions from code review

cd12345

Co-authored-by: Carlos Mocholí <[email protected]>

address comments

a442852

minor comments change

3152f81

fix tests

2c2e5ac

four4fish force-pushed the rewrite/acc_con branch from 1bf0121 to 2c2e5ac Compare February 17, 2022 19:24

four4fish requested a review from tchaton February 17, 2022 19:53

mergify bot added the has conflicts label Feb 17, 2022

Merge branch 'master' into rewrite/acc_con

71dcb82

mergify bot removed the has conflicts label Feb 17, 2022

four4fish and others added 2 commits February 17, 2022 13:44

minor fix

5f32feb

[pre-commit.ci] auto fixes from pre-commit.com hooks

f2ab1d6

for more information, see https://pre-commit.ci

ananthsub approved these changes Feb 17, 2022

View reviewed changes

mergify bot added the ready PRs ready to be merged label Feb 17, 2022

four4fish added 2 commits February 17, 2022 14:02

add _configure_launcher call to accl_conn

a6ff2c3

Merge branch 'rewrite/acc_con' of https://github.com/four4fish/pytorc…

62ce92e

…h-lightning into rewrite/acc_con

carmocca approved these changes Feb 17, 2022

View reviewed changes

four4fish and others added 2 commits February 17, 2022 14:49

Apply suggestions from code review

869e571

Co-authored-by: Carlos Mocholí <[email protected]>

Apply suggestions from code review

9568f3b

Co-authored-by: Carlos Mocholí <[email protected]>

ananthsub enabled auto-merge (squash) February 17, 2022 23:18

ananthsub merged commit 6e14209 into Lightning-AI:master Feb 17, 2022

carmocca added a commit that referenced this pull request Feb 18, 2022

Restore test after #11448

fc5211c

This was referenced Feb 18, 2022

Restore test after #11448 #11986

Merged

Fix SaveConfigCallback with ddp spawn #11987

Closed

awaelchli mentioned this pull request Feb 19, 2022

Default benchmark based on deterministic flag #11944

Merged

12 tasks

four4fish mentioned this pull request Feb 19, 2022

Add back deterministic support in accelerator_connector #11999

Merged

12 tasks

awaelchli mentioned this pull request Feb 19, 2022

Fix is_interactive_compatible logic after AcceleratorConnector rewrite #12008

Merged

11 tasks

kaushikb11 pushed a commit that referenced this pull request Feb 19, 2022

Restore test after #11448 (#11986)

6bc0e1d

justusschock pushed a commit that referenced this pull request Feb 21, 2022

Restore test after #11448 (#11986)

4f491a2

awaelchli mentioned this pull request Feb 22, 2022

Import error when trying to run TPU accelerator on CPU #12044

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite accelerator_connector #11448

Rewrite accelerator_connector #11448

four4fish commented Jan 12, 2022 •

edited

Loading

tchaton commented Jan 20, 2022

four4fish commented Jan 20, 2022

carmocca left a comment

carmocca left a comment

tchaton commented Feb 18, 2022

Rewrite accelerator_connector #11448

Rewrite accelerator_connector #11448

Conversation

four4fish commented Jan 12, 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?

tchaton commented Jan 20, 2022

four4fish commented Jan 20, 2022

carmocca left a comment

Choose a reason for hiding this comment

carmocca left a comment

Choose a reason for hiding this comment

tchaton commented Feb 18, 2022

four4fish commented Jan 12, 2022 •

edited

Loading