Skip to content

Fix various issues with test_rollout_worker.py when connector is enabled#30386

Merged
gjoliver merged 2 commits intoray-project:masterfrom
gjoliver:connector_rollout_worker_test
Nov 17, 2022
Merged

Fix various issues with test_rollout_worker.py when connector is enabled#30386
gjoliver merged 2 commits intoray-project:masterfrom
gjoliver:connector_rollout_worker_test

Conversation

@gjoliver
Copy link
Copy Markdown
Member

Signed-off-by: Jun Gong jungong@anyscale.com

Why are these changes needed?

Fix test_rollout_worker.py

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 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 :(

self.assertEqual(result["episode_reward_mean"], 1000)
# Shows different behavior when connector is on/off.
if config.enable_connectors:
# episode_reward_mean shows the correct clipped value.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Uh, nice!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where did we clip before? In algorithm?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

somewhere random man.
as soon as I see this, I was like "let's get this finished and turned on" :)

self.training_enabled = training_enabled

def step(self, action):
obs, rew, done, info = super(NoTrainingEnv, self).step(action)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This syntax was enabled with Python 3.0. I wonder why it's not part of linters for modern Python projects.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah

Copy link
Copy Markdown
Contributor

@ArturNiederfahrenhorst ArturNiederfahrenhorst left a comment

Choose a reason for hiding this comment

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

Great stuff man! Thanks a bunch 👍

Jun Gong added 2 commits November 16, 2022 19:44
…led.

Signed-off-by: Jun Gong <jungong@anyscale.com>
Signed-off-by: Jun Gong <jungong@anyscale.com>
@gjoliver gjoliver force-pushed the connector_rollout_worker_test branch from be490b2 to 469de7e Compare November 17, 2022 03:44
@gjoliver gjoliver added the tests-ok The tagger certifies test failures are unrelated and assumes personal liability. label Nov 17, 2022
@gjoliver
Copy link
Copy Markdown
Member Author

gjoliver commented Nov 17, 2022

looked at all the test failures. they are known to be broken in master.

@gjoliver gjoliver merged commit d2f250f into ray-project:master Nov 17, 2022
WeichenXu123 pushed a commit to WeichenXu123/ray that referenced this pull request Dec 19, 2022
…led (ray-project#30386)

Signed-off-by: Jun Gong <jungong@anyscale.com>
Signed-off-by: Weichen Xu <weichen.xu@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tests-ok The tagger certifies test failures are unrelated and assumes personal liability.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants