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

Add Nemotron GGUF Loading Support #34725

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

Conversation

farrosalferro
Copy link

What does this PR do?

Add Nemotron GGUF loading support

Before submitting

Who can review?

@SunMarc @LysandreJik @ArthurZucker

@@ -129,6 +129,9 @@ def load_gguf_checkpoint(gguf_checkpoint_path, return_tensors=False):
)
model_size = m.group().strip("-") # only keeps `7b`

if "nemotron" in architecture:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you explicitly assign architecture to updated one if it is the same?

Copy link
Author

Choose a reason for hiding this comment

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

I'm sorry if my answer seems obvious, but isn't it for addressing cases where the "architecture" does not only contain "nemotron"? I took reference on what you did for the qwen2moe, so I think It's better to also do it for nemotron. But I tested it without these lines and it passes through. What do you think? And thank you for reviewing! As this is my first time contributing, please let me know if anything seems odds or is there any better implementation. Thank you!

Copy link
Contributor

@VladOS95-cyber VladOS95-cyber Nov 14, 2024

Choose a reason for hiding this comment

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

No, for qwen2moe, I explicitly assigned another architecture name, because gguf file contains qwen2moe, but later, execution chain expects to get qwen2_moe for config, model processing and so on. You provided the same name "nemotron". So, there is no reason to explicitly assign updated architecture to the same name and even to mention nemotron, because gguf processing takes it from config by default.

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