-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Canary greedy and temperature decoding #8885
Conversation
Signed-off-by: Piotr Żelasko <petezor@gmail.com>
Signed-off-by: Piotr Żelasko <petezor@gmail.com>
Signed-off-by: Piotr Żelasko <petezor@gmail.com>
Signed-off-by: Piotr Żelasko <petezor@gmail.com>
@@ -103,30 +107,44 @@ | |||
self.preserve_alignments = self.cfg.get('preserve_alignments', None) | |||
self.compute_langs = self.cfg.get('compute_langs', False) | |||
self.compute_hypothesis_token_set = self.cfg.get('compute_hypothesis_token_set', False) | |||
self.transformer_decoder = transformer_decoder | |||
self.log_softmax_module = log_softmax_module | |||
self.tokenizer = tokenizer |
Check warning
Code scanning / CodeQL
Overwriting attribute in super-class or sub-class Warning
MultiTaskDecoding
nemo/collections/asr/parts/submodules/multitask_greedy_decoding.py
Dismissed
Show dismissed
Hide dismissed
): | ||
super().__init__() | ||
self.embedding = embedding | ||
self.decoder = decoder | ||
self.log_softmax = log_softmax | ||
self.log_softmax.mlp.log_softmax = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need make it work for any log_softmax
modules, we can't assume that all those modules have log_softmax.mlp.log_softmax
Signed-off-by: Piotr Żelasko <petezor@gmail.com>
self.greedy_search = GreedySequenceGenerator( | ||
embedding=transformer_decoder.embedding, | ||
decoder=transformer_decoder.decoder, | ||
log_softmax=log_softmax_module, | ||
max_sequence_length=transformer_decoder.max_sequence_length, | ||
bos=tokenizer.bos_id, | ||
pad=tokenizer.pad_id, | ||
eos=tokenizer.eos_id, | ||
max_delta_length=max_generation_delta, | ||
temperature=self.temperature, | ||
n_samples=n_samples, | ||
) |
Check failure
Code scanning / CodeQL
Wrong name for an argument in a class instantiation Error
GreedySequenceGenerator.__init__
): | ||
super().__init__() | ||
self.embedding = embedding | ||
self.decoder = decoder | ||
self.log_softmax = log_softmax | ||
self.classifier = classifier.set_log_softmax_enabled(False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add a check to see if the classifier has the set_log_softmax_enabled() function, and if not, we default to not using temperature sampling and print out a warning?
@@ -107,8 +112,8 @@ def _one_step_forward( | |||
decoder_mems_list = self.decoder.forward( | |||
decoder_hidden_states, decoder_input_mask, decoder_mems_list, return_mems=True | |||
) | |||
log_probs = self.log_softmax.forward(hidden_states=decoder_mems_list[-1][:, -1:]) | |||
return log_probs, decoder_mems_list | |||
logits = self.classifier.forward(hidden_states=decoder_mems_list[-1][:, -1:], temperature=self.temperature) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we add a check to see if the forward function has temperature
arg?
samples = list(tgt.view(orig_batch_size, self.n_samples, -1)) | ||
tgt = tgt[:: self.n_samples] | ||
|
||
return tgt, samples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're adding an additional output samples
, do we also need to update the __call__
function where the _forward
function is called?
This PR is stale because it has been open for 14 days with no activity. Remove stale label or comment or update or this will be closed in 7 days. |
This PR was closed because it has been inactive for 7 days since being marked as stale. |
What does this PR do ?
Add a one line overview of what this PR aims to accomplish.
Collection: [Note which collection this PR will affect]
Changelog
Usage
# Add a code snippet demonstrating how to use this
Jenkins CI
To run Jenkins, a NeMo User with write access must comment
jenkins
on the PR.Before your PR is "Ready for review"
Pre checks:
PR Type:
If you haven't finished some of the above items you can still open "Draft" PR.
Who can review?
Anyone in the community is free to review the PR once the checks have passed.
Contributor guidelines contains specific people who can review PRs to various areas.
Additional Information