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

Add minimal TF2 support #1026

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Add minimal TF2 support #1026

wants to merge 2 commits into from

Conversation

Miffyli
Copy link
Collaborator

@Miffyli Miffyli commented Oct 20, 2020

As per title, add minimal TF2 support while keeping TF1 support.

Motivation and Context

Closes #1012

Also motivated by the fact that TF2 offers relatively simple backwards-compatibility with TF1-like code, for now.

As noted in this comment, most of the change is just about replacing that one import. The second big thing is tensorflow.contrib, but updating for it seems reasonably doable. I am mostly worried about the hidden changes to agent performance and/or running time.

  • I have raised an issue to propose this change (required for new features and bug fixes)

TODO

  • Use tensorflow.compat.v1 to import TF1-like tensorflow.
  • Update use of tensorflow.contrib (mainly tensorflow.contrib.layers).
  • Check how training results change with this change (agent performance and training speed).

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (update in the documentation)

Checklist:

  • I've read the CONTRIBUTION guide (required)
  • I have updated the changelog accordingly (required).
  • My change requires a change to the documentation.
  • I have updated the tests accordingly (required for a bug fix or a new feature).
  • I have updated the documentation accordingly.
  • I have ensured pytest and pytype both pass (by running make pytest and make type).

@araffin
Copy link
Collaborator

araffin commented Oct 21, 2020

I would rather keep a separate branch for minimal tf2 support, as this requires tf>=1.15.

Regarding tf-contrib, all the issues are: https://github.com/Stable-Baselines-Team/stable-baselines/search?q=contrib
I'm also afraid of breaking previously saved models.
As SB2 is end-of-life, I'm not sure it is a good moment to make breaking changes like those.

@Miffyli
Copy link
Collaborator Author

Miffyli commented Oct 21, 2020

I see your points and agree this could be a separate branch, but we could inform users somehow that they should install the branch version (e.g. warning if they are importing stable-baselines with TF2.x?). As it stands stable-baselines will die once support for TF1 ends in terms of outdated libraries and whatnot, and there is bunch of research code that might rely on stable-baselines in this current form rather than stable-baselines3 and such.

@araffin
Copy link
Collaborator

araffin commented Oct 21, 2020

but we could inform users somehow that they should install the branch version

yes, in the doc and readme. For the setup.py, there is already a version limit there.

s it stands stable-baselines will die once support for TF1 ends in terms of outdated libraries and whatnot,

well, we don't plan to support SB2 forever anyway, no?

@Miffyli
Copy link
Collaborator Author

Miffyli commented Oct 21, 2020

yes, in the doc and readme. For the setup.py, there is already a version limit there.

Ok. I was thinking of cases where somebody installs TF2 afterwards, but to be fair that one is going to be on them (should follow what dependencies say).

well, we don't plan to support SB2 forever anyway, no?

Oh no, can not assume so :) . Given SB2's popularity I just would like to see a tiny bit more extension to its life-line.

@tartavull
Copy link

any progress on this?

@araffin
Copy link
Collaborator

araffin commented Oct 22, 2022

@tartavull
Copy link

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Tensorflow 2
3 participants