Constitutional AI: Harmlessness from AI Feedback

[gleibovich-nvidia]

Draft for the implementation of Constitutional AI: Harmlessness from AI Feedback.

Note that there are still some minor missing pieces regarding the exact model and datasets to be used, yet the main building blocks are in place.

[jgerh]

I completed the copyedits to the rst files identified in Constitutional AI: Harmlessness from AI Feedback
#154 and submitted them for your review. Please let me know if you have any questions.

[terrykong]

@gleibovich-nvidia Please review @jgerh 's copyedits

[gleibovich-nvidia]

I completed the copyedits to the rst files identified in Constitutional AI: Harmlessness from AI Feedback #154 and submitted them for your review. Please let me know if you have any questions.

Thanks a lot!
I have accepted and incorporated most of the suggestions. There are couple of comments, which I'm not sure about, to which I've replied directly.

[gleibovich-nvidia]

@terrykong FYI, we found an issue with the converted tokenizer being used within megatron_gpt_eval.py causing the BOS token of Mistral's prompt template not be handled correctly, when used in a conversation, rather than in a single prompt/reply. This is especially important when using few-shot prompting, as the model observes an already in progress conversation (which should follow the template correctly). It seems that megatron_gpt_eval.py is not a good fit here as an API inference service for a conversation (chat) with several rounds of human/assistant interaction, as it does not support using HF's tokenizer natively.

To resolve this issue, commits 65a9c3b and 2286522 are adding a lightweight inference service overriding the tokenizer to use the HF tokenizer instead, which tokenizes a conversation correctly.
NeMo's documentation is also mentioning using the HF tokenizer here.

[terrykong]

we found an issue with the converted tokenizer being used within megatron_gpt_eval.py causing the BOS token of Mistral's prompt template not be handled correctly

Is my understanding correct that the tokenizer within the megatron checkpoint does not tokenize the BOS as you expect?

Why not change the prompt template to use the BOS token that the tokenizer expects? I feel like maintaining a light fork of the megatron_gpt_eval.py should be a last resort.

[jgerh]

@gleibovich-nvidia Gal, I reviewed your comments, responded to your questions, and completed my review.

[gleibovich-nvidia]

we found an issue with the converted tokenizer being used within megatron_gpt_eval.py causing the BOS token of Mistral's prompt template not be handled correctly

Is my understanding correct that the tokenizer within the megatron checkpoint does not tokenize the BOS as you expect?

Why not change the prompt template to use the BOS token that the tokenizer expects? I feel like maintaining a light fork of the megatron_gpt_eval.py should be a last resort.

Yes, the BOS for Mistral models is <s> and it is then tokenized to three different tokens <, s, > with the tokenizer in the megatron checkpoint, instead of tokenizing it to just one single token <s>.

We prompt a trained instruct model (Mistral-7B-v0.1) asking for critiques and revisions. The model then expects this specific prompt template, considering the way it was trained and the data it has observed during SFT.

While I do agree it is not ideal to maintain code adapted from NeMo, I would like to stress that:

1. megatron_gpt_eval.py is merely an example of how to run inference with NeMo. AFAIU, it wasn't developed to be a comprehensive solution for LLM inference, but just an example of how it can be done for specific use-cases. It doesn't implement any combination of use-cases required by potential users. This PR, on the other hand, is requiring functionality which is missing with megatron_gpt_eval.py to get a prompt tokenized correctly to match Mistral's expected prompt template.
2. There are other examples within NeMo-Aligner where missing upstream functionality required some code adaptation and duplication. e.g. https://github.com/NVIDIA/NeMo-Aligner/blob/main/nemo_aligner/utils/train_utils.py#L16 and https://github.com/NVIDIA/NeMo-Aligner/blob/main/nemo_aligner/data/nlp/samplers.py#L28.

There might be another way to workaround this issue that I can try on Sunday morning. Let's see how that goes.
If all else fails, we can stick with this proposed workaround for now, and I will make sure to add a note highlighting that this code is adapted from NeMo, and we can remove this extra added functionality once this issue gets resolved in NeMo.

[gleibovich-nvidia]

@gleibovich-nvidia Gal, I reviewed your comments, responded to your questions, and completed my review.

Thanks a lot @jgerh!

[jgerh]

Completed reviews and updates from Gal Leibovich.

[terrykong]

Yes, the BOS for Mistral models is <s> and it is then tokenized to three different tokens <, s, > with the tokenizer in the megatron checkpoint, instead of tokenizing it to just one single token <s>.

We prompt a trained instruct model (Mistral-7B-v0.1) asking for critiques and revisions. The model then expects this specific prompt template, considering the way it was trained and the data it has observed during SFT.

I feel like I'm missing something b/c shouldn't convert_mistral_7b_hf_to_nemo.py add the tokenizer that the huggingface checkpoint was trained with? If for some reason you need a different one, I feel like the functionality should just be added to convert_mistral_7b_hf_to_nemo.py

megatron_gpt_eval.py is merely an example of how to run inference with NeMo. AFAIU, it wasn't developed to be a comprehensive solution for LLM inference, but just an example of how it can be done for specific use-cases. It doesn't implement any combination of use-cases required by potential users. This PR, on the other hand, is requiring functionality which is missing with megatron_gpt_eval.py to get a prompt tokenized correctly to match Mistral's expected prompt template.

That's true that one script may not be comprehensive, but this example seems to just be about a tokenizer being different, so I feel like that's not a big enough reason to fork. Tokenizers are usually part of the package deal for models since swapping tokenizers can usually result in garbage when the token-ids get remapped, so I prefer to address this in the model packaging/conversion rather at inference time when swapping the tokenizer is not so much a feature, but an avenue for user error.

There are other examples within NeMo-Aligner where missing upstream functionality required some code adaptation and duplication. e.g. https://github.com/NVIDIA/NeMo-Aligner/blob/main/nemo_aligner/utils/train_utils.py#L16 and https://github.com/NVIDIA/NeMo-Aligner/blob/main/nemo_aligner/data/nlp/samplers.py#L28.

Sure, but these are not examples to follow, but exceptions. Also, the second one even says it should be deleted.

In general, forking puts the burden on us to maintain the functionality and correctness. I would rather us focus on the algorithms here and push for Nemo/Megatron to incorporate our features when possible. If we ask and they say "no" or "not anytime soon", then I see that as a good reason for when we should fork.