[refactor] Make PyTorch the default and TensorFlow optional #4517
Conversation
* Adding correst setup commands for verifying torch is installed * Editing the test_requirments to add tf and remove torch
|
Edit : Removed cmdclass, the user will see an error when trying to use mlagents-learn without having torch installed. cannot directly install torch on windows via pip. |
* Torch not imported error to raise at first usage * Torch not imported error to raise at first usage
* Convert stats writer to use PyTorch TB support * Use common function to print params * Update test * Bump tensorboard to 1.15 to fix the tests * putting tensorboard 1.15.0 as min version requirement Co-authored-by: vincentpierre <vincentpierre@unity3d.com>
) * Initial commit * Forgotten doc * Removing the `Installation-Anaconda-Windows.md` as it is deprecated * Readding the depreacted Installation-Anaconda-Windows.md but leaving it unchanged * more references to tensorflow removed * Update README.md Co-authored-by: Ervin T. <ervin@unity3d.com> * Change references to .nn to .onnx in docs (#4583) Co-authored-by: Ervin T. <ervin@unity3d.com>
* Add --tensorflow option * Switch framework to Pytorch default * Update changelog * Re-add --torch * Edit warning
…ml-agents into develop-torchdefault
| self.stats_reporter.add_stat( | ||
| optimizer.reward_signals[name].stat_name, | ||
| f"Policy/{optimizer.reward_signals[name].name.capitalize()} Reward", |
There was a problem hiding this comment.
Move this to a field/property on BaseRewardProvider?
There was a problem hiding this comment.
I think that would be cleaner. @vincentpierre what do you think about this? (probably a separate PR).
| @@ -27,6 +27,7 @@ def __init__( | |||
| init_path: str = None, | |||
| multi_gpu: bool = False, | |||
| force_torch: bool = False, | |||
| force_tensorflow: bool = False, | |||
There was a problem hiding this comment.
are force_torch and force_tensorflow both needed?
There was a problem hiding this comment.
We left force_torch in case users were still using --torch in places (discussion here: #4582)
It's behavior is pretty subtle, it will only take effect if the user specifies framework: tensorflow in the YAML and then adds --torch.
| @@ -5,3 +5,4 @@ Pillow==4.2.1 | |||
| protobuf==3.6 | |||
| tensorflow==1.14.0 | |||
| h5py==2.9.0 | |||
| tensorboard==1.15.0 | |||
There was a problem hiding this comment.
Do we need this here? should be handled by setup.py.
There was a problem hiding this comment.
If we do not add it here, tensorboard version 1.14.0 is installed (because tensorflow 1.14.0 is installed here) and we do not support tensorboard 1.14.0 with torch
docs/Installation.md
Outdated
| @@ -128,6 +123,7 @@ To install the `mlagents` Python package, activate your virtual environment and | |||
| run from the command line: | |||
|
|
|||
| ```sh | |||
| pip3 install torch -f https://download.pytorch.org/whl/torch_stable.html | |||
There was a problem hiding this comment.
should we mention this is only for windows?
There was a problem hiding this comment.
It doesn't hurt on the other OS's, but I agree it's clearer if we separate windows. Updated the instructions - let me know what you think
|
Since PyTorch is default now, we should also update all .nn model files to .onnx files for the example scenes (or in separate PR?) |
@dongruoping - I have that logged as a followup in https://jira.unity3d.com/browse/MLA-1435 |
|
@ervteng is there a separate PR to update the CI configs to use |
Co-authored-by: Chris Elion <chris.elion@unity3d.com>
…ml-agents into develop-torchdefault
Yep - https://github.com/Unity-Technologies/ml-agents-cloud-internal/pull/223. It only affects the old CI, as the new CI doesn't use |
Proposed change(s)
This PR does a couple things:
torch.utils.tensorboardFurthermore, we need to verify Barracuda compatibility and package versioning before merging
Types of change(s)
Checklist
Other comments
Requires ML-Agents Cloud PR after merging