Skip to content

[rollout] fix: apply copy_to_local before init hf config #3204

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 26, 2025
Merged

[rollout] fix: apply copy_to_local before init hf config #3204

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f65317
[rollout] fix: apply copy_to_local before init hf config
Aug 25, 2025
426db98
[rollout] fix: assign new variable for local_tokenizer_path & local_h…
Aug 25, 2025
c94568f
Merge remote-tracking branch 'upstream/main' into zorn/fix/rollout
Aug 25, 2025
f39dd18
[rollout] fix: add new self variable to _mutable_fields
Aug 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions verl/workers/config/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,20 @@ def __post_init__(self):
if self.tokenizer_path is None:
self.tokenizer_path = self.path

# constuct tokenizer
self.local_path = copy_to_local(self.path, use_shm=self.use_shm)
self.tokenizer = hf_tokenizer(self.local_path, trust_remote_code=self.trust_remote_code)
self.processor = hf_processor(self.local_path, trust_remote_code=self.trust_remote_code)

self.generation_config = get_generation_config(self.hf_config_path, trust_remote_code=self.trust_remote_code)
# constuct tokenizer
local_tokenizer_path = copy_to_local(self.tokenizer_path, use_shm=self.use_shm)
self.tokenizer = hf_tokenizer(local_tokenizer_path, trust_remote_code=self.trust_remote_code)
Copy link
Collaborator

@vermouth1992 vermouth1992 Aug 25, 2025

Choose a reason for hiding this comment

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

Could you assign a variable to self? e.g., self. local_tokenizer_path

Copy link
Contributor Author

@ZornWang ZornWang Aug 25, 2025

Choose a reason for hiding this comment

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

Could you assign a variable to self? e.g., self. local_tokenizer_path

OK, I will do so.

Copy link
Contributor Author

@ZornWang ZornWang Aug 25, 2025

Choose a reason for hiding this comment

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

Could you assign a variable to self? e.g., self. local_tokenizer_path

Done, plz check if I'm doing right

Copy link
Collaborator

@vermouth1992 vermouth1992 Aug 25, 2025

Choose a reason for hiding this comment

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

Have to add to _mutable_fields as well

Copy link
Contributor Author

@ZornWang ZornWang Aug 25, 2025

Choose a reason for hiding this comment

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

Have to add to _mutable_fields as well

@vermouth1992 Done

self.processor = hf_processor(local_tokenizer_path, trust_remote_code=self.trust_remote_code)

local_hf_config_path = copy_to_local(self.hf_config_path, use_shm=self.use_shm)
self.generation_config = get_generation_config(local_hf_config_path, trust_remote_code=self.trust_remote_code)

# constuct hf_config
attn_implementation = self.override_config.get("attn_implementation", "flash_attention_2")
self.hf_config = AutoConfig.from_pretrained(
self.hf_config_path, trust_remote_code=self.trust_remote_code, attn_implementation=attn_implementation
local_hf_config_path, trust_remote_code=self.trust_remote_code, attn_implementation=attn_implementation
)
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Aug 25, 2025

Choose a reason for hiding this comment

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

high

While the fix is correct in using copy_to_local for all relevant paths, it introduces potential performance issues by calling copy_to_local multiple times for what might be the same path. If self.path, self.tokenizer_path, and self.hf_config_path point to the same remote resource, copy_to_local will be executed three times.

Although copy_to_local has caching mechanisms, it still involves overhead like hashing, file system checks, and locking, which can be costly, especially during the initialization of many workers in a distributed environment.

To avoid this redundancy and improve startup performance, you can collect all unique paths and copy each one only once.

        # Efficiently copy remote paths to local, avoiding redundant copies.
        remote_paths = {self.path, self.tokenizer_path, self.hf_config_path}
        local_path_map = {p: copy_to_local(p, use_shm=self.use_shm) for p in remote_paths}

        self.local_path = local_path_map[self.path]

        # constuct tokenizer
        local_tokenizer_path = local_path_map[self.tokenizer_path]
        self.tokenizer = hf_tokenizer(local_tokenizer_path, trust_remote_code=self.trust_remote_code)
        self.processor = hf_processor(local_tokenizer_path, trust_remote_code=self.trust_remote_code)

        local_hf_config_path = local_path_map[self.hf_config_path]
        self.generation_config = get_generation_config(local_hf_config_path, trust_remote_code=self.trust_remote_code)

        # constuct hf_config
        attn_implementation = self.override_config.get("attn_implementation", "flash_attention_2")
        self.hf_config = AutoConfig.from_pretrained(
            local_hf_config_path, trust_remote_code=self.trust_remote_code, attn_implementation=attn_implementation
        )


override_config_kwargs = {
Expand Down