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

[RLlib] Fixed the autorom dependency issue #31933

Merged

Conversation

kouroshHakha
Copy link
Contributor

Signed-off-by: Kourosh Hakhamaneshi kourosh@anyscale.com

Why are these changes needed?

Closes #31880

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

Signed-off-by: Kourosh Hakhamaneshi <kourosh@anyscale.com>
@kouroshHakha
Copy link
Contributor Author

kouroshHakha commented Jan 25, 2023

Copy link
Member

@cadedaniel cadedaniel left a comment

Choose a reason for hiding this comment

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

suggestion on wording

release/rllib_tests/app_config.yaml Outdated Show resolved Hide resolved
Co-authored-by: Cade Daniel <edacih@gmail.com>
Signed-off-by: kourosh hakhamaneshi <31483498+kouroshHakha@users.noreply.github.com>
# AutoROM downloads ROMs via torrent when they are built. The torrent is unreliable,
# so we built it for py3 and use that instead. This wheel was tested for python 3.7, 3.8,
# and 3.9.
- https://ray-ci-deps-wheels.s3.us-west-2.amazonaws.com/AutoROM.accept_rom_license-0.5.4-py3-none-any.whl
Copy link
Member

Choose a reason for hiding this comment

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

one thing, do we need to also install autorom? can't remember if this installs autorom or just the actual ROMs..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be sufficient. Let's see if the release tests pass now on ci.

@cadedaniel
Copy link
Member

@kouroshHakha
Copy link
Contributor Author

kouroshHakha commented Jan 26, 2023

@cadedaniel Do we know why the other test have not started yet? Can we increase the available compute for the release tests?

@kouroshHakha
Copy link
Contributor Author

@cadedaniel
Copy link
Member

Oh, I thought you only specified one run. Hmm capacity yeah.. did you run with high priority?

I think this should be high priority given that it's hard to get signal from many release tests until this is fixed

@kouroshHakha
Copy link
Contributor Author

Oh, I thought you only specified one run. Hmm capacity yeah.. did you run with high priority?

I think this should be high priority given that it's hard to get signal from many release tests until this is fixed

Oh I didn't set the priority. running it again. :(

@kouroshHakha
Copy link
Contributor Author

How long should it take with high priority? it's still not staring the tests?

@cadedaniel
Copy link
Member

Not sure, perhaps it is simply next in line? We might miss the nightly run tonight, it's OK.

@kouroshHakha
Copy link
Contributor Author

@cadedaniel I don't understand why these tests are still pending? should we just merge without proper testing?

@cadedaniel
Copy link
Member

Cc @krfricke do you know why the tests are still pending? Maybe we should just merge..

@kouroshHakha
Copy link
Contributor Author

Cc @krfricke do you know why the tests are still pending? Maybe we should just merge..

@krfricke just to provide more context most of the test that are pending require only 1gpu_16cpu (g3.4xlarge) machines

@kouroshHakha
Copy link
Contributor Author

@cadedaniel I think we should merge. The failures are the known ones. @ArturNiederfahrenhorst has a PR #31910 would most likely bring down the flakiness of Impala tests.

@cadedaniel
Copy link
Member

Sounds good! LGTM

@richardliaw richardliaw merged commit 76d7467 into ray-project:master Jan 27, 2023
scv119 pushed a commit that referenced this pull request Feb 1, 2023
In #31933 we fix an Atari ROM dependency that by default uses a torrent to download ROMs. The tests in this PR also break occasionally due to the same reason.

I moved the ROM dependency to S3 to increase reliability. I actually think we can remove the ROM dependency from these app configs since I don't see any RL test using them. But I think that is too much risk for this PR, since it will likely end up as a cherry pick to 2.3.
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
Co-authored-by: Cade Daniel <edacih@gmail.com>
Closes ray-project#31880

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
In ray-project#31933 we fix an Atari ROM dependency that by default uses a torrent to download ROMs. The tests in this PR also break occasionally due to the same reason.

I moved the ROM dependency to S3 to increase reliability. I actually think we can remove the ROM dependency from these app configs since I don't see any RL test using them. But I think that is too much risk for this PR, since it will likely end up as a cherry pick to 2.3.

Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
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.

[Ray release infra] RLLib dependency on Atari ROMs causing multiple release test failures
4 participants