Skip to content

revert ga fix#5030

Closed
mmathew23 wants to merge 1 commit into
unslothai:mainfrom
mmathew23:revert/ga
Closed

revert ga fix#5030
mmathew23 wants to merge 1 commit into
unslothai:mainfrom
mmathew23:revert/ga

Conversation

@mmathew23

Copy link
Copy Markdown
Contributor

No description provided.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 patch_gradient_accumulation_fix function in unsloth/models/_utils.py to use a regular expression for patching the Trainer.init method, replacing the previous string-based substitution. Feedback suggests improving the robustness of the regex to handle different quote styles (single vs. double) and simplifying the whitespace matching pattern.

Comment thread unsloth/models/_utils.py
Comment on lines +2111 to 2115
init_function = re.sub(
r'if[\s]+hasattr\(\s*unwrapped_model\s*,\s*"accepts_loss_kwargs"\s*\)\s*:',
'if hasattr(unwrapped_model, "accepts_loss_kwargs") and False:',
init_function,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The regex used for patching Trainer.__init__ is currently sensitive to the quote style used in the transformers source code (it only matches double quotes for "accepts_loss_kwargs"). To ensure the patch remains robust across different versions or environments where single quotes might be used, it's recommended to support both quote types. Additionally, [\s]+ can be simplified to \s+.

Suggested change
init_function = re.sub(
r'if[\s]+hasattr\(\s*unwrapped_model\s*,\s*"accepts_loss_kwargs"\s*\)\s*:',
'if hasattr(unwrapped_model, "accepts_loss_kwargs") and False:',
init_function,
)
init_function = re.sub(
r'if\s+hasattr\(\s*unwrapped_model\s*,\s*["\']accepts_loss_kwargs["\']\s*\)\s*:',
'if hasattr(unwrapped_model, "accepts_loss_kwargs") and False:',
init_function,
)

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