[BUG]: ChatGPT - There is a possibility that the generation is not working properly.

[brcps12]

🐛 Describe the bug

generate_kwargs is not properly passed to PPOTrainer. So, generating sentences might be incorrect. (At leat in my case)

ColossalAI/applications/ChatGPT/chatgpt/trainer/ppo.py

Lines 64 to 76 in 5d5f475

	experience_maker = NaiveExperienceMaker(actor, critic, reward_model, initial_model, kl_coef)
	replay_buffer = NaiveReplayBuffer(train_batch_size, buffer_limit, buffer_cpu_offload)
	super().__init__(strategy, experience_maker, replay_buffer, experience_batch_size, max_epochs, tokenizer,
	sample_replay_buffer, dataloader_pin_memory, callbacks, **generate_kwargs)
	self.actor = actor
	self.critic = critic
	self.actor_loss_fn = PolicyLoss(eps_clip)
	self.critic_loss_fn = ValueLoss(value_clip)
	self.actor_optim = actor_optim
	self.critic_optim = critic_optim
	self._set_default_generate_kwargs(generate_kwargs, actor)

Like Huggingface GPT2 Model, prepare_inputs_fn and update_model_kwargs_fn should be passed to generate. But in that code, these functions are not applied because _set_default_generate_kwargs() is called after super().__init__()

So, I think the order of call should be changed.

Environment

No response

[ver217]

Hi, _set_default_generate_kwargs() will modify generate_kwargs inplace, so this is OK. But I agree the order is bad and we will update this.

Thanks for your suggestion!

[brcps12]

But when super().__init__() is calling, all keyword arguments are unpacked and passed to the super class. So, I think modifying the arguments will not propagate to super class.

Like this:

class Parent:
    def __init__(self, **kwargs):
        self.kwargs = kwargs

class Child(Parent):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)

        kwargs['this_had_to_be_contained'] = 1

instance = Child(param1=0, param2=1)

# Print "{'param1': 0, 'param2': 1}"
print(instance.kwargs)

[ver217]

Can you create a PR to solve this problem? All contribution is welcome.