Fix to issue #648 #739
Fix to issue #648 #739
Conversation
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
Hi @nluu175! Thanks for the PR. I left some comments inline 🙂
Cheers,
Chris
compiler_gym/wrappers/time_limit.py
Outdated
| # def step(self, action: ActionType, **kwargs): | ||
| # assert ( | ||
| # self._elapsed_steps is not None | ||
| # ), "Cannot call env.step() before calling reset()" | ||
| # observation, reward, done, info = self.env.step(action, **kwargs) | ||
| # self._elapsed_steps += 1 | ||
| # if self._elapsed_steps >= self._max_episode_steps: | ||
| # info["TimeLimit.truncated"] = not done | ||
| # done = True | ||
| # return observation, reward, done, info |
There was a problem hiding this comment.
You can delete these lines. Don't submit commented-out code
| # done = True | ||
| # return observation, reward, done, info | ||
|
|
||
| def multistep(self, actions: Iterable[ActionType], **kwargs): |
There was a problem hiding this comment.
| def multistep(self, actions: Iterable[ActionType], **kwargs): | |
| def multistep(self, actions: Iterable[ActionType], **kwargs): | |
| actions = list(actions) |
I would suggest forcing the conversion of actions to a list here, since there could be a subtle bug if actions is an iterator, as it would break the len(actions) below.
| @@ -81,5 +81,21 @@ def test_time_limit_fork(env: LlvmEnv): | |||
| fkd.close() | |||
|
|
|||
|
|
|||
| # @pytest.mark.xfail(strict=True, reason="https://github.com/facebookresearch/CompilerGym/issues/648") | |||
There was a problem hiding this comment.
don't commit commented out code
| # @pytest.mark.xfail(strict=True, reason="https://github.com/facebookresearch/CompilerGym/issues/648") | ||
| def test_time_limit(env: LlvmEnv): | ||
| """Check CycleOverBenchmarks does not break TimeLimit""" | ||
| env = TimeLimit(env, max_episode_steps=1) |
There was a problem hiding this comment.
I would suggest testing with max_episode_steps=3, so that you can test that done is False for the first two calls to step().
| env.reset() | ||
| _, _, done, _ = env.step(0) | ||
|
|
||
| assert done |
There was a problem hiding this comment.
| assert done | |
| assert done | |
| assert info["TimeLimit.truncated"], info |
Best to also test the reason why done=True.
|
Hi @ChrisCummins . Thanks for the feedbacks. I'll resolve those and as soon as possible! |
Codecov Report
@@ Coverage Diff @@
## development #739 +/- ##
===============================================
- Coverage 88.79% 88.77% -0.03%
===============================================
Files 129 129
Lines 7854 7855 +1
===============================================
- Hits 6974 6973 -1
- Misses 880 882 +2
|
|
Changes LGTM, thanks! I will merge once CI tests pass. Cheers, |
Fixes #648