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

[Bug Report] AttributeError: accessing private attribute '_cumulative_rewards' is prohibited #877

Closed
1 task done
LetteraUnica opened this issue Jan 6, 2023 · 6 comments · Fixed by #904
Closed
1 task done
Labels
bug Something isn't working

Comments

@LetteraUnica
Copy link
Contributor

LetteraUnica commented Jan 6, 2023

Describe the bug

When creating a wrapper around an environment and overriding only the seed method, when I try to call the .last() method of the wrapped environment I get: AttributeError: accessing private attribute '_cumulative_rewards' is prohibited.
It seems the error only happens if the environment has an observation space with spaces.Sequence in it, as this does not happen on other pettinzoo environments and spaces that I tested.

I managed to re-create the issue on this colab notebook, I have also attached the same code below.

Code example

from typing import Optional, Any, Dict, List

from gymnasium import Space, spaces
from pettingzoo import AECEnv
from pettingzoo.utils import BaseWrapper


class TestEnv(AECEnv):
    metadata = {'render.modes': ['ansi'], 'name': 'test_env'}

    def __init__(self, seed: Optional[int] = None):
        super().__init__()
        self.n_cards = 52
        self.cards = []

        self.possible_agents: List[str] = ["player_0"]
        self.agents = self.possible_agents.copy()

        self.action_spaces = {agent: spaces.Discrete(self.n_cards) for agent in self.agents}
        # The bug seems to be when using spaces.Sequence
        self.observation_spaces = {agent: spaces.Sequence(spaces.Discrete(self.n_cards)) for agent in self.agents}
        self.infos = {i: {} for i in self.agents}

        self.reset(seed)

    def reset(self, seed: Optional[int] = None, return_info: bool = False, options: Optional[Dict] = None) -> None:
        self.cards = []

        self.agents = self.possible_agents.copy()
        self.rewards = {agent: 0 for agent in self.agents}
        self._cumulative_rewards = {agent: 0 for agent in self.agents}
        self.truncations = {i: False for i in self.agents}

    def seed(self, seed: Optional[int] = None) -> None:
        pass

    def observation_space(self, agent: str) -> Space:
        return self.observation_spaces[agent]

    def action_space(self, agent: str) -> Space:
        return self.action_spaces[agent]

    @property
    def agent_selection(self) -> str:
        return self.agents[0]

    @property
    def terminations(self) -> Dict[str, bool]:
        return dict([(agent, False) for agent in self.agents])

    def observe(self, agent: str) -> List[Any]:
        return self.cards

    def step(self, action: int) -> None:
        assert action in self.action_spaces[self.agent_selection]
        self.cards.append(action)

    def render(self) -> str:
        return self.cards.__repr__()

    def state(self):
        pass

    def close(self):
        super().close()


class TestEnvWrapper(BaseWrapper):
    def seed(self, seed: Optional[int] = None) -> None:
        pass



# The env works
print(TestEnv().last())

# The wrapper has an error
TestEnvWrapper(TestEnv()).last()

System info

Pettingzoo version: 1.22.3

Additional context

No response

Checklist

  • I have checked that there is no similar issue in the repo
@LetteraUnica LetteraUnica added the bug Something isn't working label Jan 6, 2023
@elliottower
Copy link
Collaborator

Are you still having this issue? Not sure why this would give you an error, as almost all of the other envs do the same thing in their code (e.g., chess: self._cumulative_rewards = {name: 0 for name in self.agents}), if it's still a problem I can look into it.

@LetteraUnica
Copy link
Contributor Author

I have changed the code since then, so I can't reproduce it on my local machine. However, I tried to re-run the colab notebook and the bug is still there. It seems to be caused by this particular observation space self.observation_spaces = {agent: spaces.Sequence(spaces.Discrete(self.n_cards)) for agent in self.agents} as other spaces that I tested and used don't cause this problem.

Edit: I deleted the previous comment and added this one as I wasn't able to add markdown code

@elliottower
Copy link
Collaborator

Thanks for the feedback. Any chance you could run a debugger on the code from colab notebook locally? (e.g., using pycharm debugger) I find that’s usually the best way to find the cause of tricky bugs like this. Otherwise I can look into it when I get a chance, but I figure you know your own code best so you might be able to better identify the problem.

@LetteraUnica
Copy link
Contributor Author

Ok I think I got it, the problem was not the observation space, as the problem persists even with other spaces.

The problem is that BaseWrapper overrides the __getattr__ method. The overridden __getattr__ method checks if an attribute starts with _, if it does then it raises an exception. On the other hand, BaseWrapper doesn't override the .last() method, so a call to .last() raises an exception as it tries to access the _cumulative_rewards attribute through the overridden __getattr__ method.

I think the problem can be fixed in three ways

  • Remove the check if name.startswith("_"): in the __getattr__ method of the BaseWrapper class.
  • Skip the check if _cumulative_rewards is accessed, for example: if name.startswith("_") and name != "_cumulative_rewards":.
  • Override the .last() method in BaseWrapper so that it accesses self.env._cumulative_rewards(), instead of utilizing the superclass method and thus accessing self._cumulative_rewards().

@jjshoots
Copy link
Member

The third options seems most reasonable to me, would you mind creating a PR for that? @LetteraUnica

@LetteraUnica
Copy link
Contributor Author

I have opened a pull request #904, however I wasn't able to run any tests as I made the changes on a windows machine, if you want I can execute the tests on linux late next week and open a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants