Skip to content

compat mcore dev#10

Closed
Jintao-Huang wants to merge 1 commit intomodelscope:mainfrom
Jintao-Huang:compat_mcore_dev
Closed

compat mcore dev#10
Jintao-Huang wants to merge 1 commit intomodelscope:mainfrom
Jintao-Huang:compat_mcore_dev

Conversation

@Jintao-Huang
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown

@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 updates the version to 1.1.0.dev0 and introduces support for variable keyword arguments in rotary positional embedding functions across gpt_model.py and patcher.py. A review comment suggests avoiding the in-place modification of the kwargs dictionary in patcher.py to prevent unintended side effects for callers.

logger.warning_once('Using non-batched RoPE, which may affect performance.')
kwargs = {'cp_group': cp_group} if mcore_013 else {}
if mcore_013:
kwargs['cp_group'] = cp_group
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Modifying the kwargs dictionary in-place can lead to unexpected side effects for the caller. It's safer to create a new dictionary for the call, which avoids mutating the object passed by the caller.

Suggested change
kwargs['cp_group'] = cp_group
kwargs = {**kwargs, 'cp_group': cp_group}

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