Tiny refactor and cleanup #75
Conversation
This reverts commit 1de0f3153db6072e9e5d4698dcf9dbcff000026f, reversing changes made to c4fa929.
|
Nice work! I'll review it later today. |
You are welcome!
Thanks!
Take your time :) |
| @@ -20,6 +20,8 @@ | |||
|
|
|||
| import os | |||
|
|
|||
| from verl.utils.config import config_normalize_batch_size | |||
There was a problem hiding this comment.
Can you move this import above L35, which would be tidier
There was a problem hiding this comment.
(will do that in the next batch after discussions of other things)
| @contextmanager | ||
| def update_sampling_params(self, **kwargs): | ||
| # update sampling params | ||
| old_sampling_params_args = {} |
There was a problem hiding this comment.
Our original implementation also used a local variable while only maintaining one SamplingParams.
It seems that your refactor introduced deepcopy.
Could you clarify the benefit of your refactor more? Thanks!
There was a problem hiding this comment.
Sure, I personally think the refactor is a little bit better, because we now avoid mutable global state, which seems to be generally not preferable in software engineering (e.g. a bit less readable and error-prone). The deepcopy is slower, but it seems the time cost is negligible, especially compared with the very heavy generate sequences.
Anyway, as the title suggests, this is just a tiny refactor, and feel free to revert it if you do not want :)
This reverts commit 0bf552d.
This is a very tiny code cleanup containing:
update_sampling_params. It originally uses a global variable and update states back and forth, and now uses immutable local variables.val_before_train: truein the default config. This is because, otherwise users has to write+trainer.val_before_train=..., while all other configs has the style oftrainer.val_before_train=...Note: This PR starts from #74, thus the GitHub diff page will be clearer after #74 is merged.(Ignore this line - that PR is already merged)