Skip to content

Conversation

@yewentao256
Copy link
Member

@yewentao256 yewentao256 commented Nov 18, 2025

Purpose

Reduce duplicate logs when using dp/tp

Signed-off-by: yewentao256 <[email protected]>
Copy link
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 optimizes startup logs to reduce duplication when running in a distributed environment (data parallelism / tensor parallelism). The changes correctly use logger.info_once and logger.warning_once with appropriate scopes (global or local) to ensure that messages are logged only once, or once per node, as intended. The implementation is correct and improves the logging behavior. For consistency and further log reduction, you might consider applying this pattern to other logger.info and logger.warning calls that are executed by multiple processes during startup.

@yewentao256 yewentao256 added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 18, 2025
Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @yewentao256!

envs.VLLM_TORCH_PROFILER_WITH_STACK,
envs.VLLM_TORCH_PROFILER_WITH_FLOPS,
)
if getattr(self.parallel_config, "data_parallel_rank", 0) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to use get_attr here since it should always be an attribute.

Also we should maybe use data_parallel_rank_local instead so that this is logged in each node?
e.g.

Suggested change
if getattr(self.parallel_config, "data_parallel_rank", 0) == 0:
if self.parallel_config.data_parallel_rank_local in (None, 0):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants