Skip to content

fix max response/context/prompt len#1110

Merged
zhuzilin merged 1 commit intoTHUDM:mainfrom
lilei199908:fix_max_prompt_len
Dec 15, 2025
Merged

fix max response/context/prompt len#1110
zhuzilin merged 1 commit intoTHUDM:mainfrom
lilei199908:fix_max_prompt_len

Conversation

@lilei199908
Copy link
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings December 15, 2025 02:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the handling of max response, context, and prompt length parameters by making them more flexible and allowing None values where appropriate.

Key Changes

  • Changed rollout_max_response_len default from 1024 to None to avoid hardcoded defaults
  • Removed automatic fallback that set rollout_max_context_len to rollout_max_response_len when None
  • Added conditional checks before setting rollout_max_prompt_len and performing related assertions to handle None values gracefully

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 1573 to 1576
logger.info(
f"args.eval_max_context_len is not set. Use args.rollout_max_context_len {args.rollout_max_context_len} as default value."
)
args.eval_max_context_len = args.rollout_max_context_len
Copy link

Copilot AI Dec 15, 2025

Choose a reason for hiding this comment

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

After removing the fallback logic that sets rollout_max_context_len to rollout_max_response_len, the code now allows rollout_max_context_len to be None. However, this block unconditionally uses args.rollout_max_context_len in both the log message and assignment without checking if it's None. This will cause issues when eval_max_context_len is set to None, which could break downstream code that expects a valid context length value. Consider adding a None check here, or only perform this assignment when rollout_max_context_len is not None.

Suggested change
logger.info(
f"args.eval_max_context_len is not set. Use args.rollout_max_context_len {args.rollout_max_context_len} as default value."
)
args.eval_max_context_len = args.rollout_max_context_len
if args.rollout_max_context_len is not None:
logger.info(
f"args.eval_max_context_len is not set. Use args.rollout_max_context_len {args.rollout_max_context_len} as default value."
)
args.eval_max_context_len = args.rollout_max_context_len
else:
raise AssertionError(
"Both eval_max_context_len and rollout_max_context_len are not set. Please set at least one of them."
)

Copilot uses AI. Check for mistakes.
@zhuzilin zhuzilin merged commit ac5328b into THUDM:main Dec 15, 2025
8 checks passed
Fengzdadi pushed a commit to Fengzdadi/slime that referenced this pull request Dec 19, 2025
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.

2 participants