Skip to content

Conversation

@JoyboyBrian
Copy link
Contributor

What does this PR do?

Clean up redundant field definitions in config dataclasses and fix a typo.

  • Remove duplicate profiler field from FSDPActorConfig (already defined in parent class ActorConfig at line 160)
  • Remove duplicate lora field from HFModelConfig (already defined at line 86 with proper type annotation dict[str, Any])
  • Fix typo in comment: constuct -> construct

Checklist Before Starting

Test

This PR only removes redundant code and fixes a typo. No functional changes.

  • profiler field: Already defined in parent class ActorConfig at line 160. FSDPActorConfig inherits from ActorConfig, so the duplicate at line 285 is unnecessary.
  • lora field: Two definitions existed in HFModelConfig:
    • Line 86: lora: dict[str, Any] = field(default_factory=dict) with comment "megatron lora config" (kept)
    • Line 91: lora: dict = field(default_factory=dict) without type parameters (removed)
    • Kept the first one because it has proper type annotation and descriptive comment.

API and Usage Example

No API changes.

Design & Code Changes

  • verl/workers/config/actor.py: Remove duplicate profiler field from FSDPActorConfig (inherited from ActorConfig:160)
  • verl/workers/config/model.py: Remove duplicate lora field (keep line 86 with dict[str, Any], remove line 91 with dict)
  • verl/workers/config/model.py: Fix typo constuct -> construct

Checklist Before Submitting

Important

Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.

  - Remove duplicate  field from FSDPActorConfig (already inherited from ActorConfig)
  - Remove  dict field from HFModelConfig (config now fully controlled by YAML)
  - Fix typo: constuct -> construct
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 provides valuable cleanup for the configuration dataclasses. It correctly removes a redundant profiler field from FSDPActorConfig that was already inherited from its parent class. More importantly, it fixes a critical bug in HFModelConfig by removing a duplicate lora field definition, which would have caused a TypeError at import time. A minor typo in a comment is also fixed. The changes are well-contained and improve the codebase's correctness and maintainability. I have reviewed the changes and found no issues.

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