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.

Sign up for GitHub

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

Jump to bottom

[RLlib] Change default framework from tf to torch #33604

Merged
merged 11 commits into from
Mar 24, 2023
Merged

[RLlib] Change default framework from tf to torch #33604

merged 11 commits into from
Mar 24, 2023

Conversation

kouroshHakha
Copy link
Contributor

Why are these changes needed?

This PR changes the default framework_str from tf to either torch or tf2. First step towards hopefully deprecating tf1 stack.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

left one out
96145c7 
Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
sven1977
sven1977 reviewed Mar 22, 2023
View reviewed changes
rllib/algorithms/algorithm_config.py
@@ -261,7 +261,7 @@ def __init__(self, algo_class=None):
self.placement_strategy = "PACK"

# `self.framework()`
self.framework_str = "tf"
self.framework_str = "torch"
Copy link
Contributor

Choose a reason for hiding this comment

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

:fingers-crossed: :)

sven1977
sven1977 approved these changes Mar 22, 2023
View reviewed changes
Copy link
Contributor

@sven1977 sven1977 left a comment

Choose a reason for hiding this comment

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

Looks great! Some example scripts used to run only on tf and will now only run on torch, but I guess that's ok. E.g. custom_metrics_and_callbacks.

@kouroshHakha
test_models fixed
4e34397 
Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
@kouroshHakha
fixed the failing tests
54add73 
Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
@kouroshHakha
Merge branch 'master' into change-default-from-tf-to-torch
55862a8
@kouroshHakha
fixed two_trainer_workflow
a1f78e2 
Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
@kouroshHakha
lint
f3a3ae8 
Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
@kouroshHakha
wip
19e60f6 
Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
gjoliver
gjoliver reviewed Mar 24, 2023
View reviewed changes
rllib/examples/documentation/saving_and_loading_algos_and_policies.py
@@ -297,6 +298,6 @@ def new_policy_mapping_fn(agent_id, episode, worker, **kwargs):

# __export-models-as-onnx-begin__
# Using the same Policy object, we can also export our NN Model in the ONNX format:
ppo_policy.export_model("/tmp/my_nn_model", onnx=True)
ppo_policy.export_model("/tmp/my_nn_model", onnx=False)
Copy link
Member

Choose a reason for hiding this comment

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

update comment?

rllib/BUILD
@@ -1618,7 +1618,7 @@ py_test(
py_test(
name = "connectors/tests/test_agent",
tags = ["team:rllib", "connector"],
size = "small",
size = "medium",
Copy link
Member

Choose a reason for hiding this comment

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

this is from some other pr right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other PR will get merged and this difference will go away. I wanted to make sure the tests on CI doesn't get red b/c of time outs.

rllib/env/tests/test_multi_agent_env.py
@@ -449,6 +449,7 @@ def compute_actions_from_input_dict(
env_creator=lambda _: MultiAgentCartPole({"num_agents": 2}),
default_policy_class=ModelBasedPolicy,
config=DQNConfig()
.framework("tf")
Copy link
Member

Choose a reason for hiding this comment

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

curious, multi-agent env doesn't work with torch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does. This test is overfitted to tf.

@gjoliver
Copy link
Member

ok ok

@gjoliver gjoliver merged commit 8d2dc9a into ray-project:master Mar 24, 2023
scottsun94 pushed a commit to scottsun94/ray that referenced this pull request Mar 28, 2023
@kouroshHakha @scottsun94
[RLlib] Change default framework from tf to torch (ray-project#33604)
e3ad926 
* changed default in algo config
* implicitly added tf framework to the test scripts

Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
cassidylaidlaw pushed a commit to cassidylaidlaw/ray that referenced this pull request Mar 28, 2023
@kouroshHakha @cassidylaidlaw
[RLlib] Change default framework from tf to torch (ray-project#33604)
d3a588b 
* changed default in algo config
* implicitly added tf framework to the test scripts

Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
brycehuang30 pushed a commit to brycehuang30/ray that referenced this pull request Mar 29, 2023
@kouroshHakha @brycehuang30
[RLlib] Change default framework from tf to torch (ray-project#33604)
66d3e50 
* changed default in algo config
* implicitly added tf framework to the test scripts

Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
Signed-off-by: bhuang <[email protected]>
joncarter1 pushed a commit to joncarter1/ray that referenced this pull request Apr 2, 2023
@kouroshHakha @joncarter1
[RLlib] Change default framework from tf to torch (ray-project#33604)
c872dac 
* changed default in algo config
* implicitly added tf framework to the test scripts

Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
Signed-off-by: Jonathan Carter <[email protected]>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
@kouroshHakha @elliottower
[RLlib] Change default framework from tf to torch (ray-project#33604)
169f986 
* changed default in algo config
* implicitly added tf framework to the test scripts

Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
Signed-off-by: elliottower <[email protected]>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
@kouroshHakha @ProjectsByJackHe
[RLlib] Change default framework from tf to torch (ray-project#33604)
ee9b0ff 
* changed default in algo config
* implicitly added tf framework to the test scripts

Signed-off-by: Kourosh Hakhamaneshi <[email protected]>
Signed-off-by: Jack He <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gjoliver gjoliver gjoliver left review comments

@sven1977 sven1977 sven1977 approved these changes

@avnishn avnishn Awaiting requested review from avnishn

@ArturNiederfahrenhorst ArturNiederfahrenhorst Awaiting requested review from ArturNiederfahrenhorst ArturNiederfahrenhorst is a code owner

@smorad smorad Awaiting requested review from smorad

@maxpumperla maxpumperla Awaiting requested review from maxpumperla

@krfricke krfricke Awaiting requested review from krfricke

Assignees

@sven1977 sven1977

@gjoliver gjoliver

@ArturNiederfahrenhorst ArturNiederfahrenhorst

@avnishn avnishn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kouroshHakha @gjoliver @sven1977 @ArturNiederfahrenhorst @avnishn