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.

Sign up for GitHub

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

Jump to bottom

expose stop words and filter eoa #352

Merged
merged 10 commits into from
Sep 26, 2023
Merged

expose stop words and filter eoa #352

merged 10 commits into from
Sep 26, 2023

Conversation

AllentDan
Copy link
Collaborator

@lvhan028 lvhan028 self-requested a review September 1, 2023 07:48
@AllentDan AllentDan mentioned this pull request Sep 1, 2023
Closed
2 tasks
@AllentDan
Copy link
Collaborator Author

@Harold-lkk We've changed the stop_words usage in this PR. Please be aware of this.

@VarunSreenivasan16
Copy link

I'm currently trying to use lmdeploy. However, I'm hitting a roadblock due to not being able to specify stop words relevant to my model. By when can this PR get merged in?

@lvhan028
Copy link
Collaborator

I'm currently trying to use lmdeploy. However, I'm hitting a roadblock due to not being able to specify stop words relevant to my model. By when can this PR get merged in?

Before next Wednesday

@AllentDan AllentDan changed the title expose stop words expose stop words and filter eoa Sep 21, 2023
@lvhan028 lvhan028 requested review from grimoire and irexyc September 21, 2023 09:39
grimoire
grimoire reviewed Sep 21, 2023
View reviewed changes
lmdeploy/model.py Outdated
@@ -539,6 +529,8 @@ def __init__(self,
elif self.capability == 'infilling':
self.top_p = kwargs.get('top_p', 0.9)
self.temperature = kwargs.get('temperature', 0.0)
if self.stop_words is None:
self.stop_words = '<EOT>'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the stop words be List[str]?

grimoire
grimoire reviewed Sep 21, 2023
View reviewed changes
lmdeploy/serve/turbomind/chatbot.py Outdated
@@ -596,6 +606,7 @@ def stream_consumer(postprocess, res_queue, session, n_input_token,
display (bool): display the text in the consolo interface or not
profile_generation (bool): indicator for profiling token generation
eos_id (int): eos token id
stop_words (List[int]): stop words and eos token id
Copy link
Collaborator

Choose a reason for hiding this comment

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

the docstring is different from the arguments of function.

grimoire
grimoire approved these changes Sep 21, 2023
View reviewed changes
Copy link
Collaborator

@grimoire grimoire left a comment

Choose a reason for hiding this comment

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

LGTM

@AllentDan AllentDan mentioned this pull request Sep 25, 2023
Closed
2 tasks
@lvhan028
Copy link
Collaborator

May update test_model.py too.
pytest tests/test_model.py raised errors

@lvhan028
Copy link
Collaborator 

  File "/data1/projects/llmdeploy/lmdeploy/utils.py", line 95, in filter_suffix
    response = response.removesuffix(item)
AttributeError: 'str' object has no attribute 'removesuffix'

removesuffix is introduced in python3.9. But we claim lmdeploy supports python >= 3.8

lvhan028
lvhan028 reviewed Sep 25, 2023
View reviewed changes
lmdeploy/model.py Outdated Show resolved Hide resolved
@AllentDan
Merge branch 'main' into stop-words
ea08b4a 
Conflicts:
	lmdeploy/model.py
@lvhan028 lvhan028 merged commit 327deae into InternLM:main Sep 26, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@irexyc irexyc irexyc approved these changes

@lvhan028 lvhan028 lvhan028 approved these changes

@grimoire grimoire grimoire approved these changes

Assignees
No one assigned
Labels
improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AllentDan @VarunSreenivasan16 @lvhan028 @grimoire @irexyc