Skip to content

Remerge release/0.2 to main#145

Closed
tastelikefeet wants to merge 19 commits intomainfrom
release/0.2
Closed

Remerge release/0.2 to main#145
tastelikefeet wants to merge 19 commits intomainfrom
release/0.2

Conversation

@tastelikefeet
Copy link
Copy Markdown
Collaborator

PR type

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

PR information

Write the detail information belongs to this PR.

Experiment results

Paste your experiment result here(if needed).

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request primarily focuses on updating the project's dependencies, versioning, and migrating various code examples and documentation to use the Qwen3_5Template instead of the generic Template. It also refactors Dockerfile package installations, integrates Megatron installations directly, and clarifies LoRA weight synchronization behavior in vLLM. Review comments highlight redundant package installations in the Dockerfile, a full-width character in English documentation, a typo in Chinese documentation, and a less deterministic version constraint for the datasets package.

RUN pip install numpy==2.2 --no-cache-dir

# Install tinker, ray, and other deps
RUN pip install --no-cache-dir tinker==0.14.0 "ray[serve]" transformers peft accelerate -U
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The packages transformers, peft, and accelerate are already installed on line 22. Installing them again here is redundant and increases the Docker image size and build time. You should remove them from this command.

RUN pip install --no-cache-dir tinker==0.14.0 "ray[serve]" -U


Currently, the model-template mapping is simple:

- Template class:Supported in all pure text LLMs.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The colon used here () is a full-width character. In English documentation, a standard colon (:) should be used for consistency with the rest of the file (e.g., line 59).

Suggested change
- Template classSupported in all pure text LLMs.
- Template class: Supported in all pure text LLMs.

- **HCCLCheckpointEngine**: 适用于昇腾 NPU 环境

> 检查点引擎是 RLHF 训练基础设施的关键组件,确保训练器和采样器使用一致的模型权重。
> 目前的同步分为merge_and_sync=True/False两种情况,为True时将lora合并仅基模并同步,为False时仅同步lora权重。另外,多租户直接附加lora文件到vLLM上,在merge_and_sync=False,或使用多租户时,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Typo: 合并仅 should be 合并进 (merged into).

Suggested change
> 目前的同步分为merge_and_sync=True/False两种情况,为True时将lora合并仅基模并同步,为False时仅同步lora权重。另外,多租户直接附加lora文件到vLLM上,在merge_and_sync=False,或使用多租户时,
> 目前的同步分为merge_and_sync=True/False两种情况,为True时将lora合并进基模并同步,为False时仅同步lora权重。另外,多租户直接附加lora文件到vLLM上,在merge_and_sync=False,或使用多租户时,

dependencies = [
"numpy>=2.0.0,<2.3.0",
"datasets>=3.0,<4.0",
"datasets",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Removing the version constraint for datasets (previously 3.0,<4.0) makes the installation less deterministic and prone to breaking if a new major version of datasets is released. It is recommended to keep a version range constraint.

Suggested change
"datasets",
"datasets>=3.0,<4.0",

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.

1 participant