Skip to content
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

Use top_p=0.0 by default (instead of 1.0) #152

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

odelalleau
Copy link
Collaborator

This is because we otherwise do useless computations, at least until
NVIDIA/NeMo#8905
is merged.

This is because we otherwise do useless computations, at least until
        NVIDIA/NeMo#8905
is merged

Signed-off-by: Olivier Delalleau <507137+odelalleau@users.noreply.github.com>
@gshennvm
Copy link
Collaborator

hm, I'm a bit hesitant to merge this because we're going to forget to revert it back again. Should we wait for your nemo PR to be merged and then cherrypick it into our dockerfile?

@odelalleau
Copy link
Collaborator Author

we're going to forget to revert it back again

We don't need to. 0.0 is fine and what we should have been using from the start (I guess we just forgot to check how it was implemented in NeMo).

@gshennvm
Copy link
Collaborator

we're going to forget to revert it back again

We don't need to. 0.0 is fine and what we should have been using from the start (I guess we just forgot to check how it was implemented in NeMo).

ah I see. then yes this LGTM. it's less confusing this way

@gshennvm
Copy link
Collaborator

could you also update the CHANGELOG to describe the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants