Asynchronous sampling vector environment #134
Conversation
|
ping @youkaichao |
|
Roger that. Keep following this pr. |
|
@youkaichao If you want to help me on that, do not hesitate. I'm quite busy right now. I can find time for reviews, but not to code something new, for at least the next 2 weeks. |
|
Ok, I'll take this pr. |
|
@duburcqa please invite me to your repo so that I can work on your branch. |
|
The head is now at b99f484 . I think we should only modify |
Yes I think so, but are you sure |
Emmm, my bad. After a second thought, I thin it is useful. |
Yes, I think both are needed. |
|
After #157 , this should be done today ✌️ |
|
Nice ! |
Codecov Report
@@ Coverage Diff @@
## dev #134 +/- ##
==========================================
- Coverage 88.94% 88.72% -0.22%
==========================================
Files 35 35
Lines 2098 2147 +49
==========================================
+ Hits 1866 1905 +39
- Misses 232 242 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@Trinkle23897 @duburcqa should be ok now, please have a look when you are available :) |
|
I approve this PR (but I cannot actually approve it since I opened it 😅). |
| simulation is disabled; else, ``1 <= wait_num <= env_num``. | ||
| """ | ||
| super().__init__(env_fns) | ||
| self.wait_num = wait_num or len(env_fns) |
There was a problem hiding this comment.
So if wait_num is None, this AsyncVenv is equal to the original SubprocVenv. Why not add this directly into the SubprocVenv?
In other words, the sync mode is a special case of async mode, thus we can unify the collector to use async mode for all venv instead of sync mode, and implement async in all venv.
There was a problem hiding this comment.
Maybe we can create another issue and pr
There was a problem hiding this comment.
Good point, and this can go further: why not just make it an ability of BaseVecEnv so that async simulation directly works for any vec envs? This seems worthwhile, but is a breaking change. Maybe we need to abstract Worker and change the interface of vec env.
There was a problem hiding this comment.
Having a unified VecEnv would be awesome and it may greatly ease the implementation of other classes/methods such as Collector.
There was a problem hiding this comment.
In other words, the sync mode is a special case of async mode, thus we can unify the collector to use async mode for all venv instead of sync mode, and implement async in all venv.
That's what I was thinking x)
There was a problem hiding this comment.
@Trinkle23897 But I think adding this feature in a dedicated class as we did is a good start I think, since it may be break changing as said.
Fix thu-ml#103 Co-authored-by: youkaichao <youkaichao@126.com> Co-authored-by: Trinkle23897 <463003665@qq.com>
Fix #103