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

Adding support for latest OLMo architectures #331

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Conversation

natolambert
Copy link
Collaborator

No description provided.

@natolambert natolambert marked this pull request as draft September 5, 2024 20:13
@natolambert natolambert marked this pull request as ready for review September 9, 2024 20:57
Copy link
Collaborator

@hamishivi hamishivi left a comment

Choose a reason for hiding this comment

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

Some comments... seems okay, but the tokenizer checks should be changed to not assume the user has hf_olmo installed.

@@ -0,0 +1,115 @@
ARG CUDA
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to add documentation about building with this image or nah? We had it before.

model_name_or_path: ai2-adapt-dev/OLMo-medium-peteish7-anneal-from-928646-50B-nowup-dclm07-flan
model_revision: main
use_flash_attn: false
tokenizer_name: ai2-adapt-dev/OLMo-medium-peteish7-anneal-from-928646-50B-nowup-dclm07-flan
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would double check with luca that this is the right tokenizer. Sometimes conversions from the olmo repo and tokenizers get jumbled...

@@ -672,7 +679,7 @@ def load_model():
0,
1,
], "LlamaTokenizer should only add one special token - the pad_token, or no tokens if pad token present."
elif isinstance(tokenizer, GPTNeoXTokenizerFast):
elif isinstance(tokenizer, GPTNeoXTokenizerFast) or isinstance(tokenizer, OLMoTokenizerFast):
Copy link
Collaborator

Choose a reason for hiding this comment

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

wont this error if olmo is not imported? Maybe we should do:
elif isinstance(tokenizer, GPTNeoXTokenizerFast) or (check_hf_olmo_availability() and isinstance(tokenizer, OLMoTokenizerFast)):

@@ -643,7 +650,7 @@ def main(args: FlatArguments):
0,
1,
], "LlamaTokenizer should only add one special token - the pad_token, or no tokens if pad token present."
elif isinstance(tokenizer, GPTNeoXTokenizerFast):
elif isinstance(tokenizer, GPTNeoXTokenizerFast) or isinstance(tokenizer, OLMoTokenizerFast): # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as above. elif isinstance(tokenizer, GPTNeoXTokenizerFast) or (check_hf_olmo_availability() and isinstance(tokenizer, OLMoTokenizerFast)):

if package_exists:
try:
# Primary method to get the package version
package_version = importlib.metadata.version(pkg_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

curious why doing this over like:

try
    import hf_olmo
 except ImportError
    ...

is preferable. Don't mind it, just curious.

Copy link
Collaborator

@vwxyzjn vwxyzjn left a comment

Choose a reason for hiding this comment

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

Looks good! Do you have a testing run?

@natolambert
Copy link
Collaborator Author

Yup @vwxyzjn have trained a couple models. Most recent one https://wandb.ai/ai2-llm/open_instruct_internal/runs/ny1m6zwx?nw=nwusernatolambert (plus another that already had eval scores, see slack)

Copy link
Collaborator

@vwxyzjn vwxyzjn left a comment

Choose a reason for hiding this comment

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

LGTM! Nice change and thanks @natolambert!

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.

4 participants