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

Removing TensorFlow Trainers #4707

Merged
merged 11 commits into from
Dec 15, 2020
Merged

Removing TensorFlow Trainers #4707

merged 11 commits into from
Dec 15, 2020

Conversation

vincentpierre
Copy link
Contributor

Proposed change(s)

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)
  • Updated the documentation (if applicable)
  • Updated the migration guide (if applicable)

Other comments

@chriselion
Copy link
Contributor

We can remove the upper limit on numpy

"numpy>=1.14.1,<1.19.0",

Which was added to keep TF happy in #4274.

@chriselion
Copy link
Contributor

We should remove the constraint files and their usage

pip_constraints: test_constraints_min_version.txt

which are used to install various versions of tensorflow during CI.

@chriselion
Copy link
Contributor

Can avoid installing tensorflow in the yamato workflow:

# TODO build these and publish to internal pypi
"~/tensorflow_pkg/tensorflow-2.0.0-cp37-cp37m-macosx_10_14_x86_64.whl",

"default framework, and will be removed in the next release.",
)
argparser.add_argument(
"--tensorflow",
default=False,
action=DetectDefaultStoreTrue,
action=RaiseDeprecationWarning,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove this one altogether? (Deprecated) usually means that it still works but will be removed in the future, whereas this flag won't do anything

Copy link
Contributor Author

@vincentpierre vincentpierre Dec 11, 2020

Choose a reason for hiding this comment

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

I see 3 options:

  • Remove --tensorflow and --torch (I think users will complain that a command line that used to work does not anymore)
  • Raise a specific error when --tensorflow is used to signal to the user that the argument has been removed
  • Only raise a warning when used (the warning can say "feature removed" instead of "feature deprecated"

I have a preference for option 2 but not a strong one. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping it around with a warning for one more release is good practice. For the wording, "tensorflow" is "removed", "--tensorflow" and "--torch" are deprecated and have no effect.

@@ -11,6 +11,7 @@ and this project adheres to
### Major Changes
#### com.unity.ml-agents (C#)
#### ml-agents / ml-agents-envs / gym-unity (Python)
- TensorFlow trainers have been removed, please use the Torch trainers instead. (#4707)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will have to move to move to the 'Unreleased' section after release 11 (unless we are aiming to get this in for the release)

Copy link
Contributor

@ervteng ervteng left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge after R11 split

@vincentpierre vincentpierre merged commit 91af77a into master Dec 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the develop-rm-tf branch December 15, 2020 17:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants