Fix usage of fs.listdir in CheckpointConnector #15413

leoleoasd · 2022-10-30T14:07:17Z

What does this PR do?

Fix use path in fs.listdir in checkpoint

Fixes #15379

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

no

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 🙃

leoleoasd · 2022-10-30T15:48:36Z

Only fixing listdir for now. Other usages of fs.xxxx still need to be discussed

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

src/pytorch_lightning/trainer/connectors/checkpoint_connector.py

leoleoasd · 2022-11-02T07:11:03Z

Hi @Borda @awaelchli any update on the review of this PR?

carmocca

There are several failing tests.

Also, a CHANGELOG entry should be added.

tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py

awaelchli · 2022-11-03T23:54:22Z

@leoleoasd Responding to your concerns. Yes, we want to support the remote filesystems. It's been added in some places of the code base, logger outputs and checkpointing are the main areas where this is supported. There was never a huge demand, but the users who do use it are mostly using the s3 protocol (from general feedback on slack). For your use cases, you did the right thing and request it on GitHub. Thanks for providing help in improving the coverage here!

tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py

…or.py Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

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

leoleoasd · 2022-11-04T14:06:49Z

Tests passed, this PR is ready for the next round of review.
The checkpointing issue is fixed, and I'll open new PR if I found some other things that doesn't work with HDFS and add necessary tests.

src/pytorch_lightning/trainer/connectors/checkpoint_connector.py

carmocca

LGTM if CI agrees

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com> Co-authored-by: otaj <6065855+otaj@users.noreply.github.com> (cherry picked from commit ee8a57d)

Fix use path in fs.listdir in checkpoint (Lightning-AI#15379)

f7aa0c5

github-actions bot added the pl Generic label for PyTorch Lightning package label Oct 30, 2022

Add tests

482260d

leoleoasd force-pushed the master branch from fe4b065 to 482260d Compare October 30, 2022 14:35

leoleoasd marked this pull request as ready for review October 30, 2022 15:48

leoleoasd requested review from awaelchli, carmocca, rohitgr7, otaj and justusschock as code owners October 30, 2022 15:48

leoleoasd and others added 2 commits October 30, 2022 23:51

Rename test & fix comments

a1bc1dd

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

e5491a8

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

awaelchli reviewed Oct 30, 2022

View reviewed changes

src/pytorch_lightning/trainer/connectors/checkpoint_connector.py Outdated Show resolved Hide resolved

awaelchli added community This PR is from the community bug Something isn't working checkpointing Related to checkpointing labels Oct 30, 2022

Borda added 2 commits November 1, 2022 01:52

Merge branch 'master' into master

5409027

Merge branch 'master' into master

843d66e

leoleoasd requested a review from awaelchli November 1, 2022 11:04

carmocca reviewed Nov 2, 2022

View reviewed changes

leoleoasd added 5 commits November 3, 2022 09:11

Fixed tests & add changelog

9403b66

Fix path

e64f9ed

Fix tests path

18b686c

Use memory fs during test

8c2685f

path instead of url in fs.isdir

f67c319

leoleoasd requested review from williamFalcon, Borda and tchaton as code owners November 3, 2022 09:07

awaelchli added 2 commits November 4, 2022 00:45

Merge branch 'master' into leoleoasd/master

1f84497

update changelog

16a9048

awaelchli changed the title ~~Fix use path in fs.listdir in checkpoint (#15379)~~ Fix usage of fs.listdir in CheckpointConnector Nov 3, 2022

awaelchli added this to the v1.8.x milestone Nov 3, 2022

awaelchli approved these changes Nov 3, 2022

View reviewed changes

tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py Outdated Show resolved Hide resolved

carmocca reviewed Nov 4, 2022

View reviewed changes

tests/tests_pytorch/trainer/connectors/test_checkpoint_connector.py Show resolved Hide resolved

leoleoasd and others added 3 commits November 4, 2022 13:02

Update tests/tests_pytorch/trainer/connectors/test_checkpoint_connect…

55764b1

…or.py Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

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

1fb4d7a

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

use os.path.join for LocalFileSystem

5949ba8

awaelchli approved these changes Nov 4, 2022

View reviewed changes

src/pytorch_lightning/trainer/connectors/checkpoint_connector.py Show resolved Hide resolved

awaelchli self-assigned this Nov 4, 2022

carmocca approved these changes Nov 4, 2022

View reviewed changes

mergify bot added the ready PRs ready to be merged label Nov 4, 2022

otaj approved these changes Nov 4, 2022

View reviewed changes

Merge branch 'master' into master

ac8ec2e

awaelchli enabled auto-merge (squash) November 4, 2022 17:16

Merge branch 'master' into master

1c5ada4

mergify bot added has conflicts and removed ready PRs ready to be merged labels Nov 4, 2022

Merge branch 'master' into leoleoasd/master

c90184e

mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Nov 4, 2022

awaelchli merged commit ee8a57d into Lightning-AI:master Nov 4, 2022

nicolai86 mentioned this pull request Dec 7, 2022

Fix typo in package publish action #15948

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix usage of fs.listdir in CheckpointConnector #15413

Fix usage of fs.listdir in CheckpointConnector #15413

leoleoasd commented Oct 30, 2022 •

edited by awaelchli

Loading

leoleoasd commented Oct 30, 2022

leoleoasd commented Nov 2, 2022

carmocca left a comment

awaelchli commented Nov 3, 2022

leoleoasd commented Nov 4, 2022

carmocca left a comment

Fix usage of fs.listdir in CheckpointConnector #15413

Fix usage of fs.listdir in CheckpointConnector #15413

Conversation

leoleoasd commented Oct 30, 2022 • edited by awaelchli 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?

leoleoasd commented Oct 30, 2022

leoleoasd commented Nov 2, 2022

carmocca left a comment

Choose a reason for hiding this comment

awaelchli commented Nov 3, 2022

leoleoasd commented Nov 4, 2022

carmocca left a comment

Choose a reason for hiding this comment

leoleoasd commented Oct 30, 2022 •

edited by awaelchli

Loading