Enhance rslora comment clarity across notebooks and scripts #144
Conversation
Summary of ChangesHello @Sneakr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the clarity and usability of the codebase by updating documentation comments. Specifically, it provides explicit instructions for setting the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the clarity of the rslora configuration comment across a large number of Jupyter notebooks and Python scripts. The change adds a helpful tip about setting alpha when use_rslora is enabled and standardizes the term to 'rank-stabilized LoRA'. While the change is beneficial, the resulting comment makes many lines exceed the recommended length, impacting readability. I've added suggestions to shorten the comment for better adherence to style guidelines.
| " use_gradient_checkpointing = \"unsloth\", # True or \"unsloth\" for very long context\n", | ||
| " random_state = 3407,\n", | ||
| " use_rslora = False, # We support rank stabilized LoRA\n", | ||
| " use_rslora = False, # We support rank-stabilized LoRA (If True, set alpha ~ sqrt(r) for correct scaling)\n", |
There was a problem hiding this comment.
The code within this notebook cell string contains a very long line, which can harm readability when viewed as code. To improve this, I suggest shortening the comment. This suggestion applies to all other notebooks modified in this PR.
| " use_rslora = False, # We support rank-stabilized LoRA (If True, set alpha ~ sqrt(r) for correct scaling)\n", | |
| " use_rslora = False, # RsLoRA: For correct scaling, set alpha ~ sqrt(r) if True.\n", |
| use_gradient_checkpointing = "unsloth", # True or "unsloth" for very long context | ||
| random_state = 3407, | ||
| use_rslora = False, # We support rank stabilized LoRA | ||
| use_rslora = False, # We support rank-stabilized LoRA (If True, set alpha ~ sqrt(r) for correct scaling) |
There was a problem hiding this comment.
This line is quite long (over 110 characters), which exceeds the common recommendation of 88 characters per line from style guides like PEP 8 / black. Long lines can be harder to read. To improve readability, I suggest shortening the comment. This suggestion is applicable to all other Python scripts modified in this PR.
| use_rslora = False, # We support rank-stabilized LoRA (If True, set alpha ~ sqrt(r) for correct scaling) | |
| use_rslora = False, # RsLoRA: For correct scaling, set alpha ~ sqrt(r) if True. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves comment clarity for rslora and loftq_config parameters across a large number of notebooks and Python scripts. The new comment for rslora is a significant improvement, providing useful guidance on its usage. The changes are consistent and improve code readability and maintainability. However, a critical issue has been identified: the loftq_update_log.txt file, which appears to be a local script execution log, has been added. This file contains a local file path, posing a security concern, and should be removed. For better clarity, I also suggest updating the pull request title and description to reflect all the changes made, including the updates to loftq_config comments.
loftq_update_log.txt
Outdated
| Comment updated to: We support LoftQ | ||
| Run at: 2025-12-18T02:47:41 | ||
| Root: C:\Users\HZ\Documents\Dev\unsloth\notebooks |
There was a problem hiding this comment.
This log file appears to be a generated artifact from a local script and should not be committed to the repository.
It contains sensitive information, such as the local file path on your machine on line 3: C:\\Users\\HZ\\Documents\\Dev\\unsloth\\notebooks. Exposing such information is a security risk.
Please remove this file from the pull request. To prevent this from happening again, consider adding loftq_update_log.txt or a more general pattern like *.txt or *.log to your .gitignore file.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request systematically improves the clarity of configuration options across a large number of notebooks and Python scripts. The primary change enhances the comment for the use_rslora parameter, providing crucial information about scaling alpha. Additionally, the changes include stylistic improvements such as standardizing comments for loftq_config and correcting spacing and alignment, which significantly boosts code readability and maintainability. The updates are consistent and well-executed. Great work!
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to enhance the clarity of the rslora comment across numerous notebooks and Python scripts. While the change is an improvement, there's a discrepancy between the comment added and the one mentioned in the pull request description. The description suggests (If True, set alpha ~ sqrt(r) for correct scaling), which provides a clearer rule for setting the alpha parameter. The implemented comment uses a hardcoded range ~16-32, which is less informative. I've added suggestions to a notebook and a Python script to illustrate how to use the more descriptive comment. This change should be applied to all modified files.
| " random_state = 3407,\n", | ||
| " use_rslora = False, # We support rank stabilized LoRA\n", | ||
| " loftq_config = None, # And LoftQ\n", | ||
| " use_rslora = False, # If True, set alpha to ~16–32\n", |
There was a problem hiding this comment.
The new comment is an improvement, but the pull request description mentions (If True, set alpha ~ sqrt(r) for correct scaling), which is more descriptive and helpful than the hardcoded range ~16-32. It explains the relationship between alpha and r, providing better guidance. It's recommended to use this more informative comment across all modified files.
| " use_rslora = False, # If True, set alpha to ~16–32\n", | |
| " use_rslora = False, # If True, set alpha ~ sqrt(r) for correct scaling\n", |
| random_state = 3407, | ||
| use_rslora = False, # We support rank stabilized LoRA | ||
| loftq_config = None, # And LoftQ | ||
| use_rslora = False, # If True, set alpha to ~16–32 |
There was a problem hiding this comment.
As with the notebook files, the comment here could be more descriptive. The pull request description suggests (If True, set alpha ~ sqrt(r) for correct scaling), which is more informative than the hardcoded range ~16-32. This will help users understand how to set the alpha parameter correctly.
| use_rslora = False, # If True, set alpha to ~16–32 | |
| use_rslora = False, # If True, set alpha ~ sqrt(r) for correct scaling |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the clarity of the use_rslora parameter's comment across a large number of notebooks and Python scripts. The new comment provides practical, concrete ranges for the lora_alpha (α) hyperparameter, which is a great improvement for users. The changes also include some nice stylistic cleanups, like adding PEP8-compliant spacing and making related comments more consistent.
I've left a couple of specific comments with a suggestion to also include the α ≈ sqrt(r) rule of thumb, as mentioned in your PR description. This would provide valuable context by linking the practical advice to the theoretical foundation from the Rank-Stabilized LoRA paper. This suggestion applies to all files changed in this PR.
Additionally, please note the discrepancy between the PR description, which mentions adding (If True, set alpha ~ sqrt(r) for correct scaling), and the actual change. Updating the PR description to match the final implementation would improve clarity. There's also a small typo in the description ('clarify' instead of 'clarity').
Overall, this is a valuable change that improves the usability and consistency of these examples.
| " random_state = 3407,\n", | ||
| " use_rslora = False, # We support rank stabilized LoRA\n", | ||
| " loftq_config = None, # And LoftQ\n", | ||
| " use_rslora = False, # If True, use α≈16–32 for high ranks; α≈4–16 for low ranks.\n", |
There was a problem hiding this comment.
The new comment provides helpful, concrete ranges for α. To provide even more context for users, consider also mentioning the α ≈ sqrt(r) rule of thumb from the Rank-Stabilized LoRA paper, as you noted in the pull request description. This would connect the practical advice with the underlying principle. This suggestion can be applied to all similar changes in the notebooks, preserving the original value of use_rslora.
| " use_rslora = False, # If True, use α≈16–32 for high ranks; α≈4–16 for low ranks.\n", | |
| " use_rslora = False, # If True, use α ≈ sqrt(r). Eg: α≈16–32 for high ranks; α≈4–16 for low ranks.\n", |
| random_state = 3407, | ||
| use_rslora = False, # We support rank stabilized LoRA | ||
| loftq_config = None, # And LoftQ | ||
| use_rslora = False, # If True, use α≈16–32 for high ranks; α≈4–16 for low ranks. |
There was a problem hiding this comment.
The new comment provides helpful, concrete ranges for α. To provide even more context for users, consider also mentioning the α ≈ sqrt(r) rule of thumb from the Rank-Stabilized LoRA paper, as you noted in the pull request description. This would connect the practical advice with the underlying principle. This suggestion can be applied to all similar changes in the Python scripts, preserving the original value of use_rslora.
| use_rslora = False, # If True, use α≈16–32 for high ranks; α≈4–16 for low ranks. | |
| use_rslora = False, # If True, use α ≈ sqrt(r). Eg: α≈16–32 for high ranks; α≈4–16 for low ranks. |
1 participant
Adds
(If True, set alpha ~ sqrt(r) for correct scaling)to rslora comments for clarify.