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

Add docs and graceful Mujoco cleanup ( #1224) #1283

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

a-ayesh
Copy link

@a-ayesh a-ayesh commented Dec 28, 2024

Description

A detailed comment is provided in my name under the issue referenced.

The change summary is:

  • Added AttributeError exception handling to the free() method in mujoco_rendering.py.
  • Gracefully handles freeing the OpenGL context and closing the GLFW window in the event that the user does not call env.close() before the program terminates when they've set render_mode: 'human'.
  • Added docstring for the free() method.

Fixes #1224

Type of change

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

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

Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts 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 the PR, I think this will resolve the issue for users.
Could you change the print to a gymnasium.logger.warn
Also could you run pre-commit run --all-files

Otherwise good to me, @Kallinteris-Andreas any thoughts?

glfw.make_context_current(None)
glfw.destroy_window(self.window)
self.window = None
except AttributeError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can a more explicitly error type other than AttributeError be used?

If not, it is fine

Copy link
Author

Choose a reason for hiding this comment

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

Let me look into it. Maybe there is another error which can be caught earlier in the stack trace.

Copy link
Author

Choose a reason for hiding this comment

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

Nope. This is the exact error it throws. No other error is thrown or handled before it.

I did try to use the sys lib to handle it more gracefully, but it gets unloaded before glfw thus resulting in an import error which is more confusing.

Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas left a comment

Choose a reason for hiding this comment

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

It works, tested with:

import gymnasium

env = gymnasium.make('Humanoid-v5', render_mode='human')
obs, info = env.reset()
#env.close()

Also, please run pre-commit --all-files (more info on contributing.md)

@a-ayesh
Copy link
Author

a-ayesh commented Dec 31, 2024

Could you change the print to a gymnasium.logger.warn

I changed the print statement to use warn. The error now looks like:

/home/user/Gymnasium/gymnasium/envs/mujoco/mujoco_rendering.py:372: UserWarning: WARN: Environment was not properly closed using 'env.close()'. Please ensure to close the environment explicitly. GLFW module or dependencies are unloaded. Window cleanup might not have completed.

Also could you run pre-commit run --all-files

Also I apologize for marking that I ran pre-commit run --all-files. I am actually struggling to run it? I get stuck at

An unexpected error has occurred: CalledProcessError: command: ('/home/cowlar/.cache/pre-commit/repoiipfy4zl/node_env-system/bin/node', '/usr/local/bin/npm', 'install', '--include=dev', '--include=prod', '--ignore-prepublish', '--no-progress', '--no-save')
return code: 1
stdout: (none)
stderr:
    internal/modules/cjs/loader.js:638
        throw err;
        ^
    
    Error: Cannot find module 'node:path'
        at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
        at Function.Module._load (internal/modules/cjs/loader.js:562:25)
        at Module.require (internal/modules/cjs/loader.js:692:17)
        at require (internal/modules/cjs/helpers.js:25:18)
        at Object.<anonymous> (/usr/local/lib/node_modules/npm/lib/cli.js:10:18)
        at Module._compile (internal/modules/cjs/loader.js:778:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
        at Module.load (internal/modules/cjs/loader.js:653:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
        at Function.Module._load (internal/modules/cjs/loader.js:585:3)
Check the log at /home/cowlar/.cache/pre-commit/pre-commit.log

I am using my work PC though, so I don't want to mess up anything just in case. It would be nice if either of you could checkout the branch from my fork to run it, otherwise I might do it in a day or two.

@Kallinteris-Andreas
Copy link
Collaborator

You can use GitHub Codespaces on your branch to easily run pre-commit.
Or use pipx locally

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.

[Question] 'NoneType' object has no attribute 'glfwGetCurrentContext'
4 participants