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 DeepSeek template and template register #814

Merged
merged 2 commits into from
May 7, 2024

Conversation

wheresmyhair
Copy link
Collaborator

Add DeepSeek template and template register
Also solves the template init error when using python >= 3.11 (due to @DataClass decorator)

Copy link
Contributor

@research4pan research4pan left a comment

Choose a reason for hiding this comment

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

LGTM. The register operation can be made simpler for easier understanding and maintenance.

src/lmflow/models/hf_decoder_model.py

  • [Style] line 463: recommend to put line 470 in front of line 463. Unlikely events come later, which is better for understanding, and sometimes performance.

src/lmflow/utils/conversation_template.py

  • [Style, Architecture] line 13, 295-317: the registration function can be put in a different file, such as utils/template/custom.py or utils/template/base.py.
    • For standard templates, we can define different templates first, then directly map names to templates in line 13. This way the code is cleaner and easier to extend. In current styles, if we would like to add more arguments to templates, register_template has to adapt accordingly, which will be a problem in maintenance.
    • For customized templates, we can use this register function, with some examples provided in README.

@research4pan research4pan merged commit 6c69834 into main May 7, 2024
2 checks passed
@wheresmyhair wheresmyhair deleted the yizhenjia-deepseek-template branch May 8, 2024 08:01
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.

None yet

2 participants