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

Video recording utilities #1499

Merged
merged 34 commits into from
Nov 21, 2022
Merged

Video recording utilities #1499

merged 34 commits into from
Nov 21, 2022

Conversation

AisenGinn
Copy link
Contributor

Added gif_recorder.py and recorder_wrapper.py into smarts.env.wrappers folder for video recording function.

@Gamenot Gamenot changed the title Develop Video recording utilities Jul 12, 2022
Copy link
Collaborator

@Gamenot Gamenot left a comment

Choose a reason for hiding this comment

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

Please also remove the SMARTS submodule included in this branch.

smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/recorder_wrapper.py Outdated Show resolved Hide resolved
smarts/env/wrappers/recorder_wrapper.py Outdated Show resolved Hide resolved
smarts/env/wrappers/recorder_wrapper.py Outdated Show resolved Hide resolved
@Gamenot
Copy link
Collaborator

Gamenot commented Aug 23, 2022

@AisenGinn Please bug people to review this so it does not sit too long.

smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/gif_recorder.py Outdated Show resolved Hide resolved
smarts/env/wrappers/recorder_wrapper.py Outdated Show resolved Hide resolved
smarts/env/wrappers/recorder_wrapper.py Outdated Show resolved Hide resolved
@AisenGinn AisenGinn requested review from Gamenot and saulfield and removed request for saulfield, qianyi-sun and Gamenot November 17, 2022 19:36
Copy link
Collaborator

@Gamenot Gamenot left a comment

Choose a reason for hiding this comment

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

One set of issues to figure out. Good Work!

Comment on lines 29 to 33
try:
from moviepy.editor import ImageClip, ImageSequenceClip
except (ImportError, ModuleNotFoundError):
print(sys.exc_info())
print(
"You may not have installed the [gym] dependencies required to capture the video. Install them first using the command `pip install -e .[gym]` at the source directory."
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think my last concern here is that this try/catch swallows the import error. I think we still want to raise an error here. Aside from that we should not expect that the user is working from a dev platform.

Suggested change
try:
from moviepy.editor import ImageClip, ImageSequenceClip
except (ImportError, ModuleNotFoundError):
print(sys.exc_info())
print(
"You may not have installed the [gym] dependencies required to capture the video. Install them first using the command `pip install -e .[gym]` at the source directory."
)
try:
from moviepy.editor import ImageClip, ImageSequenceClip
except (ImportError, ModuleNotFoundError):
logging.warning(sys.exc_info())
logging.warning(
"You may not have installed the [gym] dependencies required to capture the video. Install them first with the `smarts[gym]` extras."
)
raise

Copy link
Collaborator

@Gamenot Gamenot left a comment

Choose a reason for hiding this comment

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

Good work!

@Gamenot Gamenot merged commit d302972 into huawei-noah:develop Nov 21, 2022
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