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

Replace NumPy with Torch in examples/fabric/ #17279

Merged

Conversation

ryan597
Copy link
Contributor

@ryan597 ryan597 commented Apr 4, 2023

What does this PR do?

Refactoring to remove all numpy function calls in the examples/fabric folder.

Part of Issue: #16649

Related to Pull Requests: #17264, #17267 and #17278

Removal of numpy package import in the listed files.
Most replacements are simple substitutions of np.<func> to torch.<func>, but also the inclusion of logical_not() where subtraction with tensors of bools are not allowed.
Removal of seeding numpy rng

Where .numpy() is called, it has been changed to .tolist() as input to external functions.
.tolist() is much slower than .numpy() and since .numpy() is part of the torch package this doesn't require the numpy import.

Replace Tensor(...).to(device) with torch.tensor(..., device=device)

Files:

  • examples/fabric/meta_learning/train_fabric.py
  • examples/fabric/meta_learning/train_torch.py
  • examples/fabric/reinforcement_learning/rl/agent.py
  • examples/fabric/reinforcement_learning/rl/utils.py
  • examples/fabric/reinforcement_learning/train_torch.py
  • examples/fabric/reinforcement_learning/train_fabric.py
  • examples/fabric/reinforcement_learning/train_fabric_decoupled.py

Part of #16649

Before submitting
  • Was this discussed/agreed 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 verify new and existing tests pass locally with your changes?
  • 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 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:

Reviewer checklist
  • 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

@github-actions github-actions bot added fabric lightning.fabric.Fabric pl Generic label for PyTorch Lightning package labels Apr 4, 2023
@ryan597 ryan597 marked this pull request as ready for review April 5, 2023 13:58
@ryan597
Copy link
Contributor Author

ryan597 commented Apr 5, 2023

lightning-fabric (GPUs) (testing pkg: fabric) is failing at the standalone test for strategies/test_fsdp_integration.py.
Link to pipeline fail

I'm not sure how to go about resolving the failed test as the examples I changed aren't going into that test..

@github-actions github-actions bot removed the pl Generic label for PyTorch Lightning package label Apr 5, 2023
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

LGTM :)
minor comments regarding the creation of tensors.

examples/fabric/reinforcement_learning/train_torch.py Outdated Show resolved Hide resolved
examples/fabric/reinforcement_learning/train_torch.py Outdated Show resolved Hide resolved
examples/fabric/reinforcement_learning/train_fabric.py Outdated Show resolved Hide resolved
examples/fabric/reinforcement_learning/rl/agent.py Outdated Show resolved Hide resolved
@awaelchli awaelchli added refactor community This PR is from the community labels Apr 6, 2023
examples/fabric/reinforcement_learning/rl/utils.py Outdated Show resolved Hide resolved
examples/fabric/reinforcement_learning/rl/agent.py Outdated Show resolved Hide resolved
@awaelchli awaelchli added this to the 2.1 milestone Apr 6, 2023
@awaelchli
Copy link
Contributor

I'm not sure how to go about resolving the failed test as the examples I changed aren't going into that test..

They are unrelated, you can ignore them and we will rerun them before merging.

@awaelchli awaelchli added the ready PRs ready to be merged label Apr 6, 2023
ryan597 and others added 11 commits April 6, 2023 20:51
change numpy functions to the torch counterparts
remove seeding numpy rng
remove the numpy import
replace np.array in layer_init calls
remove numpy import
replace the np.sqrt in layer init default arg
replace np.logical_or with built-in or, no need for tensors/array.
remove numpy import
subtraction was not valid for tensor of bools. Replace with logical not
same changes for all files
remove numpy import
Subtraction with tensors of bools is not supported.
tolist() is significantly slower than numpy()
Part of the torch so still allows for removal of numpy import
also to(device) moved within the tensor creation where possible
tensor(..., device=device)
remove "from torch import Tensor" in some files where not needed
@ryan597 ryan597 force-pushed the refactor/16649_numpy_to_torch branch from 9bb27c9 to aa49d64 Compare April 6, 2023 19:51
@justusschock justusschock merged commit fb775e0 into Lightning-AI:master Apr 6, 2023
@ryan597 ryan597 deleted the refactor/16649_numpy_to_torch branch April 6, 2023 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR is from the community fabric lightning.fabric.Fabric ready PRs ready to be merged refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants