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

fix custom vllm eval args #2325

Merged
merged 4 commits into from
Oct 23, 2024
Merged

fix custom vllm eval args #2325

merged 4 commits into from
Oct 23, 2024

Conversation

Yunnglin
Copy link
Collaborator

@Yunnglin Yunnglin commented Oct 23, 2024

PR type

  • Bug Fix
  • New Feature
  • Document Updates
  • More Models or Datasets Support

PR information

Fix eval dataset with vllm inference backend args.
Add following in ms-swift/swift/llm/utils/vllm_utils.py

if self.repetition_penalty is None:
    self.repetition_penalty = 1.0
if self.top_p is None:
    self.top_p = 1.0
if self.top_k is None:
    self.top_k = -1

Experiment results

Paste your experiment result here(if needed).

@Jintao-Huang
Copy link
Collaborator

vllm_utils.py可以还原一下不, 怎么改了这么多哇

@@ -260,12 +270,24 @@ def __post_init__(self):
self.top_k = -1
if self.stop is None:
self.stop = []
if self.repetition_penalty is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

可以在eval.py中对None进行过滤不

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@Yunnglin
Copy link
Collaborator Author

vllm_utils.py可以还原一下不, 怎么改了这么多哇

自动lint工具修改的,改了一些格式,vllm_utils.py还原了

@@ -620,5 +620,6 @@ def prepare_vllm_engine_template(args: InferArguments, use_async: bool = False)
args.truncation_strategy,
model=llm_engine,
tools_prompt=args.tools_prompt)
logger.info(f'system: {template.default_system}')
args.system = template.default_system
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是为什么呢

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was likely modified by mistake and has been restored.

if max_new_tokens is not None:
infer_cfg['max_tokens'] = max_new_tokens
defaults = {'repetition_penalty': 1.0, 'top_p': 1.0, 'top_k': -1}
# 使用默认值覆盖 None 值
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use english

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@Jintao-Huang
Copy link
Collaborator

thank you for your PR!

@Yunnglin Yunnglin merged commit 8bf0bb4 into main Oct 23, 2024
2 of 3 checks passed
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.

3 participants