-
Notifications
You must be signed in to change notification settings - Fork 183
Add video recording to the job #84
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for putting together this PR! I think this is a great start for setting up headless rendering. But I think we can make some changes to make the overall setup cleaner:
-
the agent should not have to care about the video recorder. The video recording logic should be handled by the environment and engine.
-
lets move the video recorded as an object inside the engine. When creating the engine, we can pass a "record_video" flag, which will tell the engine whether or not to create the video recorder.
-
only record videos during testing phase. The environment has a flag, which the agent sets to tell the environment if it's in the training phase or the testing phase:
MimicKit/mimickit/envs/base_env.py
Line 20 in 7d0aa9e
self._mode = EnvMode.TRAIN
The environment can use this flag to determine if it's currently in the testing phase, and only record videos during test. -
Is it possible to not rely on the environment's camera logic for the video recorder? For example, we could add some simple camera controls into the recorder, then this will remove the dependency on the environment's camera controls. It would be ideal, if the only thing the environment has to do is to turn on the engine's video recorder when testing begins, and turn off the recorder when testing ends. Everything else can be handled by the engine.
-
is it necessary to have a maximum length for the video? If we are only recording videos during the testing phase, we can end the video whenever the testing phase ends.
Hope these make sense! Let me know if you have any questions. I can also take a crack at making these changes too. Let me know what I can help with.
mimickit/learning/base_agent.py
Outdated
| self._mode = AgentMode.TRAIN | ||
| self._curr_obs = None | ||
| self._curr_info = None | ||
| self._video_recorder = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be cleaner if the agent doesn't need to know about the video recorder. Is it possible to refactor this code so that the video recorder is a object inside the engine? Then the engine can handle all the logic for video recording. The only thing that the env will have to do is check if the engine has video recording enabled, and update the camera if needed. Then all the logic of when to call the recorder will be handled by the engine.
mimickit/learning/base_agent.py
Outdated
| def _rollout_train(self, num_steps): | ||
| for i in range(num_steps): | ||
| if (self._video_recorder is not None): | ||
| self._video_recorder.pre_step() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably only want to record videos during the testing phase not during the training phase. Since the policy will be very jittery during the training phase. It will be more informative to record during the testing phase instead.
requirements.txt
Outdated
| tensorboardX | ||
| torch>=1.9.1 | ||
| wandb>=0.17.4 | ||
| setuptools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I encountered an error without this. But I am not able to reproduce it now. I guess it must have depended on some setup sequences. nvm, I removed it for now unless I can reproduce it again.
mimickit/run.py
Outdated
| return | ||
|
|
||
| def build_video_recorder(args, env): | ||
| video = args.parse_bool("video", False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move the building of the video recorder into the engine. Since we'll probably need different recorder classes for different engines.
mimickit/learning/base_agent.py
Outdated
| self._iter += 1 | ||
|
|
||
| # flush any in-progress video recording at end of training | ||
| if (self._video_recorder is not None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (self._video_recorder is not None): | |
| if self._video_recorder: |
Since _video_recorder is not numerical type with zero value, we can safely do this simpler form instead
f9f7583 to
0cd84a7
Compare
Add 3 flags to control video rendering:
It renders a video during test_model
The video is first stored in
wandb/{job_name}/files/media/videos/, and synced to wandb.In wandb individual runs, you can see different snapshots by scrolling the process bar:

In the group run view you can see multiple runs comparison:

Limitation: