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] Incorrect behaviour from __getattr__ in OrderEnforcingWrapper? #1174

Open
1 task done
dm-ackerman opened this issue Feb 1, 2024 · 0 comments · May be fixed by #1205
Open
1 task done

[Bug Report] Incorrect behaviour from __getattr__ in OrderEnforcingWrapper? #1174

dm-ackerman opened this issue Feb 1, 2024 · 0 comments · May be fixed by #1205
Labels
bug Something isn't working

Comments

@dm-ackerman
Copy link
Contributor

Describe the bug

I'm getting an error from OrderEnforcingWrapper when trying to access env.action_spaces.

It comes from __getattr__, which I included in the code section for context.

I see several issues with that function as it's written:

  1. value == "unwrapped" will never be triggered because the base class has an unwrapped property which returns the same result
  2. value == "render_mode" duplicates the effect of the base class's __getattr__.
  3. both "observation_spaces" and "action_spaces" have a misleading message (referring to possible_agents), and more importantly, don't check whether the base class has the attribute before raising an error claiming it doesn't exist.
  4. Lastly, all of the cases except for the last elif statement are not related to the stated goal of the docstring and the intent of the class (as I understand it).

The chained wrappers are a bit unclear to me and it looks like there was already some work related to this on #1140 , so I wanted to see if I'm misunderstanding anything before submitting a change.

I'd like to address the items above as follows:

  1. remove this
  2. remove this
  3. return the spaces if they exist, error otherwise; fix the text of the error
  4. move all except last elif to BaseWrapper's __getattr__

Code example

def __getattr__(self, value: str) -> Any:
    """Raises an error message when data is gotten from the env.

    Should only be gotten after reset
    """
    if value == "unwrapped":
        return self.env.unwrapped
    elif value == "render_mode" and hasattr(self.env, "render_mode"):
        return self.env.render_mode  # pyright: ignore[reportGeneralTypeIssues]
    elif value == "possible_agents":
        try:
            return self.env.possible_agents
        except AttributeError:
            EnvLogger.error_possible_agents_attribute_missing("possible_agents")
    elif value == "observation_spaces":
        raise AttributeError(
            "The base environment does not have an possible_agents attribute. Use the environments `observation_space` method instead"
        )
    elif value == "action_spaces":
        raise AttributeError(
            "The base environment does not have an possible_agents attribute. Use the environments `action_space` method instead"
        )
    elif value == "agent_order":
        raise AttributeError(
            "agent_order has been removed from the API. Please consider using agent_iter instead."
        )
    elif (
        value
        in {
            "rewards",
            "terminations",
            "truncations",
            "infos",
            "agent_selection",
            "num_agents",
            "agents",
        }
        and not self._has_reset
    ):
        raise AttributeError(f"{value} cannot be accessed before reset")
    else:
        return super().__getattr__(value)

System info

>>> import sys; sys.version
'3.9.12 (main, Apr  5 2022, 06:56:58) \n[GCC 7.5.0]'
>>> pettingzoo.__version__
'1.24.3'

Additional context

No response

Checklist

  • I have checked that there is no similar issue in the repo
@dm-ackerman dm-ackerman added the bug Something isn't working label Feb 1, 2024
@dm-ackerman dm-ackerman linked a pull request May 6, 2024 that will close this issue
7 tasks
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.

1 participant