Skip to content

Conversation

@otaj
Copy link
Contributor

@otaj otaj commented Aug 15, 2022

What does this PR do?

Since #12981 and #13640, the new mechanism for re-instantiating DataLoader and BatchSamplers was adopted in case it was necessary to modify some of their behavior. While in general, this allowed for greater flexibility, it also introduced a subtle bug, that it was impossible to set attributes on these classes that would survive the re-instantiation process. To illustrate, let's take a look at the example below:

loader = DataLoader(...)
loader.my_attribute = 10 # <- this will not survive re-instantiation

In order to fix it, this PR patches __setattr__ and __delattr__ methods, which serve as tape recorders, recording all the __setattr__ and __delattr__ calls happening on the object while in the proper context manager. During re-instantiation, all of these calls are replayed on top of the newly created object.

It also replaces all calls of setattr(obj, <internal_pl_name>, value) to object.__setattr__(obj, <internal_pl_name>, value) , especially within these contexts in order to not run into RecursionErrors

It was discovered together with @krshrimali while debugging Flash CI

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 @Borda @justusschock @awaelchli @ninginthecloud @rohitgr7 @otaj

@otaj otaj added bug Something isn't working data handling Generic data-related topic labels Aug 15, 2022
@otaj otaj added this to the pl:1.7.x milestone Aug 15, 2022
@otaj otaj self-assigned this Aug 15, 2022
@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Aug 15, 2022
@awaelchli
Copy link
Contributor

This is a huge rabbit hole, what are we doing 🤣 🤣
What's next? Let's also record all method calls on the dataloader and replaying them? They could have side effects outside the dataloader 🤯 Where will we stop? 😅

@otaj
Copy link
Contributor Author

otaj commented Aug 16, 2022

I fully agree, @awaelchli, that we're digging a rabbit hole here. And, to be fair, I was also thinking of recording the calls 😂

But, this was sadly a real issue, experienced by Flash and I'd rather have this solved for everyone and somewhat nicely, than telling @krshrimali hacks to make it work without this PR 😂

Copy link
Contributor

@krshrimali krshrimali left a comment

Choose a reason for hiding this comment

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

Thanks, @otaj for the PR! 🚀 LGTM

@mergify mergify bot added the ready PRs ready to be merged label Aug 16, 2022
@otaj otaj enabled auto-merge (squash) August 16, 2022 07:49
Copy link
Collaborator

@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.

stretched my knowledge a bit, but so far I can say, LGTM

Copy link
Contributor

@carmocca carmocca left a comment

Choose a reason for hiding this comment

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

Did not review. Will check it in the future. Approving to unblock 👍

edit: LGTM!

@otaj otaj merged commit 44cdbca into master Aug 17, 2022
@otaj otaj deleted the bugfix/_wrap_setattr branch August 17, 2022 15:42
awaelchli pushed a commit that referenced this pull request Aug 17, 2022
awaelchli added a commit that referenced this pull request Aug 17, 2022
…stantiated inside `*_dataloader` hooks (#14212)

Co-authored-by: otaj <[email protected]>
lexierule pushed a commit that referenced this pull request Aug 17, 2022
* update version and changelog for 1.7.2 release

* Reset all results on epoch end (#14061)

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

* Skip ddp fork tests on windows (#14121)

* Fix device placement when `.cuda()` called without specifying index (#14128)

* Convert subprocess test to standalone test (#14101)

* Fix entry point test for Python 3.10 (#14154)

* Fix flaky test caused by weak reference (#14157)

* Fix saving hyperparameters in a composition where parent is not a LM or LDM (#14151)



Co-authored-by: Rohit Gupta <[email protected]>

* Remove DeepSpeed version restriction from Lite (#13967)

* Configure the check-group app (#14165)

Co-authored-by: Jirka <[email protected]>

* Update onnxruntime requirement from <=1.12.0 to <1.13.0 in /requirements (#14083)

Updates the requirements on [onnxruntime](https://github.com/microsoft/onnxruntime) to permit the latest version.
- [Release notes](https://github.com/microsoft/onnxruntime/releases)
- [Changelog](https://github.com/microsoft/onnxruntime/blob/master/docs/ReleaseManagement.md)
- [Commits](microsoft/onnxruntime@v0.1.4...v1.12.1)

---
updated-dependencies:
- dependency-name: onnxruntime
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update gcsfs requirement from <2022.6.0,>=2021.5.0 to >=2021.5.0,<2022.8.0 in /requirements (#14079)

Update gcsfs requirement in /requirements

Updates the requirements on [gcsfs](https://github.com/fsspec/gcsfs) to permit the latest version.
- [Release notes](https://github.com/fsspec/gcsfs/releases)
- [Commits](fsspec/gcsfs@2021.05.0...2022.7.1)

---
updated-dependencies:
- dependency-name: gcsfs
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fix a bug that caused spurious `AttributeError` when multiple `DataLoader` classes are imported (#14117)


fix

* CI: Replace `_` of in GHA workflow filenames with `-` (#13917)

* Rename workflow files

* Update docs

* Fix azure badges

* Update the main readme

* bad rebase

* Update doc

* CI: Update Windows version from 2019 to 2022 (#14129)

Update windows

* CI/CD: Add CUDA version to docker image tags (#13831)

* append cuda version to tags

* revertme: push to hub

* Update docker readme

* Build base-conda-py3.9-torch1.12-cuda11.3.1

* Use new images in conda tests

* revertme: push to hub

* Revert "revertme: push to hub"

This reverts commit 0f7d534.

* Revert "revertme: push to hub"

This reverts commit 46a05fc.

* Run conda if workflow edited

* Run gpu testing if workflow edited

* Use new tags in release/Dockerfile

* Build base-cuda and PL release images with all combinations

* Update release docker

* Update conda from py3.9-torch1.12 to py3.10-torch.1.12

* Fix ubuntu version

* Revert conda

* revertme: push to hub

* Don't build Python 3.10 for now...

* Fix pl release builder

* updating version contribute to the error? docker/buildx#456

* Update actions' versions

* Update slack user to notify

* Don't use 11.6.0 to avoid bagua incompatibility

* Don't use 11.1, and use 11.1.1

* Update .github/workflows/ci-pytorch_test-conda.yml

Co-authored-by: Luca Medeiros <[email protected]>

* Update trigger

* Ignore artfacts from tutorials

* Trim docker images to distribute

* Add an image for tutorials

* Update conda image 3.8x1.10

* Try different conda variants

* No need to set cuda for conda jobs

* Update who to notify ipu failure

* Don't push

* update filenaem

Co-authored-by: Luca Medeiros <[email protected]>

* Avoid entry_points deprecation warning (#14052)

Co-authored-by: Adam J. Stewart <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>

* Configure the check-group app (#14165)

Co-authored-by: Jirka <[email protected]>

* Profile batch transfer and gradient clipping hooks (#14069)

Co-authored-by: Rohit Gupta <[email protected]>

* Avoid false positive warning about using `sync_dist` when using torchmetrics (#14143)

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

* Avoid raising the sampler warning if num_replicas=1 (#14097)

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

Co-authored-by: otaj <[email protected]>

* Remove skipping logic in favor of path filtering (#14170)

* Support checkpoint save and load with Stochastic Weight Averaging (#9938)

Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <[email protected]>
Co-authored-by: Carlos Mocholi <[email protected]>
Co-authored-by: Kushashwa Ravi Shrimali <[email protected]>
Co-authored-by: Jirka <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>

* Use fsdp module to initialize precision scalar for fsdp native (#14092)

Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Laverne Henderson <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>

* add more issues types (#14174)

* add more issues types

* Update .github/ISSUE_TEMPLATE/config.yml

Co-authored-by: Mansy <[email protected]>

* typo

Co-authored-by: Adrian Wälchli <[email protected]>

Co-authored-by: Kaushik B <[email protected]>
Co-authored-by: Mansy <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>
Co-authored-by: Laverne Henderson <[email protected]>
Co-authored-by: Akihiro Nitta <[email protected]>

* CI: clean building docs (#14216)

* CI: clean building docs

* group

* .

* CI: docker focus on PL only (#14246)

* CI: docker focus on PL only

* group

* Allowed setting attributes on `DataLoader` and `BatchSampler` when instantiated inside `*_dataloader` hooks (#14212)


Co-authored-by: otaj <[email protected]>

* Revert "Remove skipping logic in favor of path filtering (#14170)" (#14244)

* Update defaults for WandbLogger's run name and project name (#14145)

Co-authored-by: Carlos Mocholí <[email protected]>
Co-authored-by: Rohit Gupta <[email protected]>
Co-authored-by: Jirka <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Akihiro Nitta <[email protected]>
Co-authored-by: Luca Medeiros <[email protected]>
Co-authored-by: Adam J. Stewart <[email protected]>
Co-authored-by: otaj <[email protected]>
Co-authored-by: Adam Reeve <[email protected]>
Co-authored-by: thomas chaton <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Kushashwa Ravi Shrimali <[email protected]>
Co-authored-by: Laverne Henderson <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Kaushik B <[email protected]>
Co-authored-by: Mansy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working data handling Generic data-related topic pl Generic label for PyTorch Lightning package ready PRs ready to be merged

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

7 participants