Skip to content
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 35 additions & 40 deletions examples/seq2seq/seq2seq_trainer.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,13 @@


class Seq2SeqTrainer(Trainer):
def __init__(self, config, data_args, *args, **kwargs):
def __init__(self, *args, **kwargs):

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@patil-suraj @sshleifer - I think it would be better to align the init of Seq2SeqTrainer 100% with Trainer.
Is there a reason why we would insert config instead of using the model's config?

Also I don't really think the variable data_args is necessary. Both max_length and num_beams can be defined in the config and don't have to be "force" passed to the generate() method.

@patil-suraj patil-suraj Oct 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model.config breaks under DistributedDataParallel, so we decided to pass it explicitly. See #7461 and #7460.

if default num_beams and max_length is too high it'll slow down evaluation, so we allow the user to control it during training. And not overriding config since defaults will be needed for inference after training.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okok I see! I'm a bit confused why Trainer does not break with DistributedDataParallel when only using model.config.... , but Seq2SeqTrainer does? Do you guys know why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eval_beams/eval_max_gen_length reasoning:
@patil-suraj said exactly this LOL, but in my words:
users are not good at modifying configs locally. We want to have a way to run num_beams=2 during the generation step, but then end up with a trained model with the default # beams. In general, we try not to manipulate config attributes that would only be desired during training.

I mean modifying the configs locally is as simple as config.num_beams = 4 and I would think one wants to evaluate a model during training with exactly the beam size and max_length that is stored in the config (I mean changing the beam size and max_length does not simple reduce time, but also changes the output...) But I guess I can see the use case where the people want to tweak max_length and num_beams without changing the config. Would it be fine to make data_args optional and call them generation_args that will just be passed as **generation_args to the generate function?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ups that was supposed to land further below not here. @sshleifer for reference.

@sshleifer sshleifer Oct 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

data_args -> generation_kwargs seems like a good change (at least in seq2seq_trainer.py), but the CLI naming has a purpose:
It wouldn't have been obvious to me that passing --min_length 32 would affect generation, rather than truncating source docs. That's why the eval_ prefix was added.

super().__init__(*args, **kwargs)
self.config = config
self.data_args = data_args
self.max_gen_length = data_args.val_max_target_length
self.vocab_size = self.config.tgt_vocab_size if isinstance(self.config, FSMTConfig) else self.config.vocab_size
self.vocab_size = (
self.model.config.tgt_vocab_size
if isinstance(self.model.config, FSMTConfig)
else self.model.config.vocab_size
)

def create_optimizer_and_scheduler(self, num_training_steps: int):
"""
Expand Down Expand Up @@ -114,23 +115,22 @@ def _get_train_sampler(self) -> Optional[torch.utils.data.sampler.Sampler]:
else DistributedSampler(self.train_dataset)
)

def compute_loss(self, model, inputs):
labels = inputs.pop("labels")
outputs = model(**inputs, use_cache=False)
logits = outputs[0]
return self._compute_loss(logits, labels)

def _compute_loss(self, logits, labels):
def _compute_loss(self, model, inputs):
if self.args.label_smoothing == 0:
# Same behavior as modeling_bart.py
loss_fct = torch.nn.CrossEntropyLoss(ignore_index=self.config.pad_token_id)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to work for all models (EncoderDecoderModel does not work with it) -> Let's instead use the loss function of each model here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loss functions of model use -100 as ignore_index , we will also need to replace pad tokens in labels with -100

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually do this manually before -> should that be the role of the Seq2SeqTrainer? Trainer also does not have this feature

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignoring pad_token_id confused lots of people and helps metrics so we automated it.
Related: #7828

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually do this manually before

we could do this in the collator, but we won't need to do if #7828 is merged

@sshleifer sshleifer Oct 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will still need to cover FSMT/T5.
I would definitely not do this change right now, it works as is and is much easier than checking that every model ignores padding.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyTorch's CE loss function has -100 as a default value and from what I understood it is the default behavior of the library to ignore tokens when there have the index -100 and not when there are equal to the padding token (often we set padding token == -100): https://pytorch.org/docs/stable/generated/torch.nn.CrossEntropyLoss.html

It would require models to manually replace tokens with -100, but I think that's how it should be done in general in the library. How would be handle models that don't have a padding_token or want to disregard loss of more than just the padding token? For such cases I think it can be quite handy if the user overwrites all labels he does not want to consider with -100

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will discuss on zoom!

assert logits.shape[-1] == self.vocab_size
loss = loss_fct(logits.view(-1, logits.shape[-1]), labels.view(-1))
# compute usual loss via models
loss, logits = model(**inputs)[:2]
Comment thread
sshleifer marked this conversation as resolved.
Outdated
else:
# compute label smoothed loss
labels = inputs.pop("labels")
logits = model(**inputs, use_cache=False)[0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think use_cache=False everywhere or nowhere

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it - think it's better this way to not give the false impression that use_cache=True will break training. All models have use_cache=True by default and training works by default. It's all about whether past_key_values are inserted or not.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this actually breaks a test - it shouldn't. This is related to this Bart bug we never solved: #6353 :-/

@patrickvonplaten patrickvonplaten Oct 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add use_cache=False again for now and remove it when fixing the bug in Bart.

lprobs = torch.nn.functional.log_softmax(logits, dim=-1)
loss, nll_loss = label_smoothed_nll_loss(
lprobs, labels, self.args.label_smoothing, ignore_index=self.config.pad_token_id
loss, _ = label_smoothed_nll_loss(
lprobs, labels, self.args.label_smoothing, ignore_index=self.model.config.pad_token_id
)
return loss, logits

def compute_loss(self, model, inputs):
Comment thread
sshleifer marked this conversation as resolved.
loss, _ = self._compute_loss(model, inputs)
return loss

def prediction_step(
Expand Down Expand Up @@ -158,34 +158,29 @@ def prediction_step(
"""
inputs = self._prepare_inputs(inputs)

if self.args.predict_with_generate and not self.args.prediction_loss_only:
generated_tokens = model.generate(
inputs["input_ids"],
attention_mask=inputs["attention_mask"],
)
# in case the batch is shorter than max length, the output should be padded
generated_tokens = self._pad_tensors_to_max_len(generated_tokens, self.model.config.max_length)

# compute loss on predict data
with torch.no_grad():

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generate() is always in torch.no_grad() context.

if self.args.predict_with_generate and not self.args.prediction_loss_only:
generated_tokens = model.generate(
inputs["input_ids"],
attention_mask=inputs["attention_mask"],
use_cache=True,
num_beams=self.data_args.eval_beams,
max_length=self.max_gen_length,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need this if eval_beams and and max_length are different than default

)
# in case the batch is shorter than max length, the output should be padded
generated_tokens = self._pad_tensors_to_max_len(generated_tokens, self.max_gen_length)
loss, logits = self._compute_loss(model, inputs)
Comment thread
patrickvonplaten marked this conversation as resolved.

labels_out = inputs.get("labels")
# Call forward again to get loss # TODO: avoidable?
outputs = model(**inputs, use_cache=False)
loss = self._compute_loss(outputs[1], labels_out)
loss = loss.mean().detach()
if self.args.prediction_loss_only:
return (loss, None, None)
loss = loss.mean().detach()
if self.args.prediction_loss_only:
return (loss, None, None)

logits = generated_tokens if self.args.predict_with_generate else outputs[1]
logits = generated_tokens if self.args.predict_with_generate else logits
labels = self._pad_tensors_to_max_len(inputs["labels"], self.model.config.max_length)

labels_out = labels_out.detach()
labels = self._pad_tensors_to_max_len(labels_out, self.max_gen_length)
return (loss, logits.detach(), labels)
return (loss, logits, labels)

def _pad_tensors_to_max_len(self, tensor, max_length):
padded_tensor = self.config.pad_token_id * torch.ones(
padded_tensor = self.model.config.pad_token_id * torch.ones(
(tensor.shape[0], max_length), dtype=tensor.dtype, device=tensor.device
)
padded_tensor[:, : tensor.shape[-1]] = tensor
Expand Down