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

update gym -> gymnasium, ray=2.0.0 -> ray=2.5.0 #153

Closed
wants to merge 1 commit into from
Closed

update gym -> gymnasium, ray=2.0.0 -> ray=2.5.0 #153

wants to merge 1 commit into from

Conversation

dimonenka
Copy link
Contributor

  • I tested my local (modified) rllib/self_play_train.py
  • I did not test utils_test.py
  • I did not test render
  • Upgrading ray requires using gymnasium instead of gym, which petingzoo also relies on. However, newer versions of petingzoo also use gymnasium. I updated petingzoo example but didn't test. Chances are, this example will work with little changes, but needs testing and updating to the newer versions of petingzoo in the requirements.

Not ideal but hope it still helps.

@google-cla
Copy link

google-cla bot commented Jul 24, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@jagapiou jagapiou self-assigned this Jul 25, 2023
Copy link
Member

@jagapiou jagapiou left a comment

Choose a reason for hiding this comment

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

Thanks for this. Added some comments, I don't know ray very well so they may well be ignorant :-)

FYI the pytype and test-examples actions are broken, I've got a fix incoming.

examples/rllib/self_play_train.py Outdated Show resolved Hide resolved
examples/requirements.txt Outdated Show resolved Hide resolved
examples/pettingzoo/utils.py Show resolved Hide resolved
examples/rllib/utils.py Show resolved Hide resolved
examples/gym/utils.py Outdated Show resolved Hide resolved
@jagapiou
Copy link
Member

FYI: got some updates to the requirements landing soon (chex<0.1.81, unpinned tensorflow) and a fix of some of the utils_test.py.

@jagapiou jagapiou self-requested a review July 25, 2023 15:53
Copy link
Member

@jagapiou jagapiou left a comment

Choose a reason for hiding this comment

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

One more change and then it's all good to go in.

Please squash your commits into one.

Thank you!

@jagapiou
Copy link
Member

self._num_players not self.num_players please.

@dimonenka
Copy link
Contributor Author

sorry for the bugs, trying to squash now

@dimonenka
Copy link
Contributor Author

I'm not sure if I can squash:

  • squashing seems to only work for the last n commits, meaning it would affect the commits of other collaborators
  • I had to merge with main and merge commits are not allowed when squashing

@dimonenka
Copy link
Contributor Author

dimonenka commented Jul 26, 2023

Should I create a new pull request with all commits in one?

@jagapiou
Copy link
Member

jagapiou commented Jul 26, 2023

I think you should be able to do this to squash it all into one commit while keeping this PR:

git tag pr153  # for safety only
git fetch upstream
git reset --soft upstream/main
git commit -m 'update gym -> gymnasium, ray=2.0.0 -> ray=2.5.0'
git push --force

@dimonenka
Copy link
Contributor Author

Yea I don't know, it didn't work, says there are no changes to commit

@jagapiou
Copy link
Member

Ah sorry forgot to specify reset to upstream/main. I've updated the instructions.

@dimonenka
Copy link
Contributor Author

This is magic, you must be a wizard.

@jagapiou
Copy link
Member

jagapiou commented Jul 26, 2023

Sorry, a few changes that weren't in your branch got reverted we should have merged upstream/main into main before squashing.

Option 1

Manually revert them and then fix and then use

git commit --amend --no-edit
git push --force

Option 2

Assuming you tagged as pr315 as above (hooray for safety tags), I think you can just git reset --hard pr153 to get back to where you were and then try again with:

git fetch upstream
git merge upstream/main  # the thing I forgot
git reset --soft upstream/main
git commit -m 'update gym -> gymnasium, ray=2.0.0 -> ray=2.5.0'
git push --force

@dimonenka
Copy link
Contributor Author

Yep hard reset worked

@dimonenka
Copy link
Contributor Author

Automatic tests always failed to install gymnasium -- is that ok?

.github/workflows/pypi-publish.yml Outdated Show resolved Hide resolved
.github/workflows/pypi-test.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@jagapiou
Copy link
Member

Fantastic, thanks so much.

When I drop the petting zoo stuff from examples/requirements.txt the gymnasium issues went away so I think the automatic tests are failing because of a dependency conflict between pettingzoo and ray. So there's still more work updating these examples. But this has gotten us off to a good start :-)

@jagapiou
Copy link
Member

I've trigged our pipeline that pulls this PR to our internal repo, checks it breaks nothing internally, asks someone to LGTM it, and then it will hopefully hit the public repo soon.

@dimonenka
Copy link
Contributor Author

Cool, thanks! Let me know if something else is needed on my end.

copybara-service bot pushed a commit that referenced this pull request Jul 26, 2023
--
607e87d by dimonenka <dimonenka@mail.ru>:

update gym -> gymnasium, ray=2.0.0 -> ray=2.5.0

COPYBARA_INTEGRATE_REVIEW=#153 from dimonenka:main 607e87d
PiperOrigin-RevId: 551208054
Change-Id: I4b50f929d0369eb38e2e0e7b4bcd012b43b198eb
@jagapiou
Copy link
Member

Merged in a72757f

@jagapiou jagapiou closed this Jul 26, 2023
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.

None yet

2 participants