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 fix: Revert an incorrect edition on wrapper.FrameStack #2973

Merged
merged 2 commits into from
Jul 18, 2022

Conversation

ZhiqingXiao
Copy link
Contributor

@ZhiqingXiao ZhiqingXiao commented Jul 17, 2022

Description

#2752 incorrectly removes the obligated positional parameter of self.observation(), which makes the wrapper class no longer work. This bug fix corrects it.

@arjun-kg @pseudo-rnd-thoughts @jkterry1

Type of change

Please delete options that are not relevant.

  • [ +] Bug fix (non-breaking change which fixes an issue)

Screenshots

Please attach before and after screenshots of the change if applicable.

Before fix:

File ~\anaconda3\envs\py310\lib\site-packages\gym\wrappers\frame_stack.py:181, in FrameStack.step(self, action)
    176 observation, reward, terminated, truncated, info = step_api_compatibility(
    177     self.env.step(action), True
    178 )
    179 self.frames.append(observation)
    180 return step_api_compatibility(
--> 181     (self.observation(), reward, terminated, truncated, info), self.new_step_api
    182 )

TypeError: FrameStack.observation() missing 1 required positional argument: 'observation'

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

openai#2752 incorrectly removes the obligated positional parameter. This bug fix corrected it.
@pseudo-rnd-thoughts
Copy link
Contributor

Thanks for your PR, could you run the pre-commit, CONTRIBUTIONS.md in the root folder for an explanation of implementation.

@ZhiqingXiao
Copy link
Contributor Author

Unfortunately I had difficulty in setting up the environment. I might give up.
I still suggest merge this PR even when there is a check failure, since it does not make things worse.

Detail:
I installed git and pre-commit, and executed the pre-commit command, but I encountered the error "ImportError: DLL load failed while importing _ctypes: The specified module could not be found." in the trace of "import ctypes". I checked that in that Anaconda environment both ctypes and _ctypes can be successfully imported and used, so there are something complex/weired happened. I Googled, but it seems that there are no clear solution for it.

The error message when running pre-commit run --all-files:

      File "C:\Users\Zhiqing\.cache\pre-commit\repoilsxiwyi\py_env-default\lib\site-packages\pip\_vendor\platformdirs\windows.py", line 3, in <module>
        import ctypes
      File "C:\Users\Zhiqing\anaconda3\envs\py310\lib\ctypes\__init__.py", line 8, in <module>
        from _ctypes import Union, Structure, Array
    ImportError: DLL load failed while importing _ctypes: The specified module could not be found.

@pseudo-rnd-thoughts
Copy link
Contributor

Could you update it to

        return step_api_compatibility(
            (self.observation(None), reward, terminated, truncated, info),
            self.new_step_api,
        )

Reformat the limit the line width.
@ZhiqingXiao
Copy link
Contributor Author

Could you update it to

        return step_api_compatibility(
            (self.observation(None), reward, terminated, truncated, info),
            self.new_step_api,
        )

Thanks very much. I edited accordingly. Now it passed all checks.

@pseudo-rnd-thoughts
Copy link
Contributor

LGTM

@jkterry1 jkterry1 merged commit ce6bad7 into openai:master Jul 18, 2022
@ZhiqingXiao
Copy link
Contributor Author

Thanks.

@ZhiqingXiao ZhiqingXiao deleted the frame_stack_bugfix branch July 19, 2022 13:57
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.

3 participants