Concerns Regarding Recent PettingZoo PRs #17375
Comments
|
Hey @jkterry1, @rodrigodelazcano, Thanks for raising this to our attention! We did not have any malicious intent when doing this and moving forward, we will absolutely try to communicate better. Also, congratulations on the maintainership for OpenAI Gym! Let me try to provide some intent and context for these incidents. I want to emphasize that both of these PRs that you pointed out as reverted actually already landed on master at least a week ago. 1) Reverting a multi-agent config change #17036 Cause: The original PR was posted by @sven1977 from our team, and aimed to improve the multi-agent configuration for RLlib (linked here: #16565). From what I understand, this PR identified that tests for PettingZoo API support were never run in the first place and upgraded pistonball_v1 to pistonball_v4. This original PR was done entirely on our side (without any Pettingzoo team involvement). @sven1977 merged this PR, but it ended up breaking a test on the Ray CI. It is our team policy to revert ASAP to maintain a robust build. Thus, @amogkam reverted this PR and notified @sven1977 regarding this revert. Followup: the original PR landed again #17046 1 day after. There was never a degradation of PettingZoo compatibility on a released version on Ray. However, we can definitely tag you and @rodrigodelazcano upon any PZ relevant changes moving forward. 2) Reverting pettingzoo RPS #16886 Cause: We reverted @rodrigodelazcano's RPS pull request because it broke the master build as part of the PR. Followup: We actually already merged a fix to restore the PR last week: #16896. Here, I definitely appreciate your feedback and understand that we should have notified you / @rodrigodelazcano ASAP of the revert. Please accept our apologies here. We'll be sure to communicate more effectively next time! @jkterry1 Do you have anything else you'd like us to do differently next time? |
|
Hey, I apologize coming off strong, if this never impacted master/production then it's no harm no fowl as far as I'm concerned. This looks like it came down to a communication issue. We may all just be blind, but when me and the three people I work with looked at those PRs we thought things were just being stripped out. If you guys had made a comments like "removing, will readd soon because ___" and linked those PRs to the other PRs so it showed up on the PR in GitHub, then we would've understood what was going on and wouldn't have cared. Making these things very clear in general is probably a good idea independent of this, because I assume we aren't the only people trying to follow these things? You certainly don't have to notify us for minor things, which these appear to be from your description, though notifying us if large changes come up in the future would be greatly appreciated so that we're able to take a look. Again, I apologize for coming off strong here. |
|
No problem at all! Thanks for all that you’re doing in the ecosystem.
…On Tue, Jul 27, 2021 at 7:13 PM J K Terry ***@***.***> wrote:
Hey,
I apologize coming off strong, if this never impacted master/production
then it's no harm no fowl as far as I'm concerned. This looks like it came
down to a communication issue. We may all just be blind, but when me and
the three people I work with looked at those PRs we thought things were
just being stripped out. If you guys had made a comments like "removing,
will readd soon because ___" and linked those PRs to the other PRs so it
showed up on the PR in GitHub, then we would've understood what was going
on and wouldn't have cared. Making these things very clear in general is
probably a good idea independent of this, because I assume we aren't the
only people trying to follow these things?
You certainly don't have to notify us for minor things, which these appear
to be from your description, though notifying us if large changes come up
in the future would be greatly appreciated so that we're able to take a
look.
Again, I apologize for coming off strong here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17375 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABCRZZPETDZZSJ6D7K6WD3DTZ5RWJANCNFSM5BDEQLJA>
.
|
Per 38b5b6d#diff-6e6f5ac309686c42bec61e8cf74c48e92895c1abaf1b57a60679f105670ee42d you guys removed tests for PettingZoo API support, and (I assume accidentally) broke things in the process by downgrading from pistonball_v4 to pistonball_v1 in the process. PettingZoo is now the third most installed RL library on PyPI (after Gym and RLlib) (https://pepy.tech/project/pettingzoo) with more than 20 open source third party environments and more in the work (most are here: https://github.com/PettingZoo-Team/PettingZoo/network/dependents?dependents_before=MTUyMTMxMjc4MjY&package_id=UGFja2FnZS0xMTQ4MzIxNzgy), and every opensource MARL library expect Tianshou (which is working on adding support) supports it. As such, ensuring the proper function of PettingZoo in RLlib is important for both you guys and us. Moreover, your PR contained no explanation of what happened and you guys didn't notify us about changes, which raises reliability and professional courtesy concerns. Could we please come to an amicable solution here? E.g. fixing tests and pistonball version history and ensuring that we're notified about future changes like this?
Moreover, per ecb6321#diff-6e6f5ac309686c42bec61e8cf74c48e92895c1abaf1b57a60679f105670ee42d you reverted a contribution of ours without comment or notification. The reason we created it is that, in our opinion, if there's a widely used RPS implementation out in the wild that's supported by your library it makes more sense for a maintenance and factoring standpoint than to include your own, but you may disagree and this ultimately isn't highly consequential. However, again there's the professional courtesy issue of just letting us know what's happening when you revert someone's PR in full.
This is a side note, but OpenAI recently made me the maintainer of Gym in large part due to the widespread popularity of PettingZoo so I can merge maintenance PRs from you guys there as applicable- openai/gym#2259. We should be working together here guys.
@amogkam @richardliaw cc @rodrigodelazcano
The text was updated successfully, but these errors were encountered: