fix: fsdp_config validation being None#3061
Conversation
📝 WalkthroughWalkthroughRefactors validation utilities to safely access a possibly missing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/axolotl/utils/schemas/validation.py(2 hunks)
🔇 Additional comments (1)
src/axolotl/utils/schemas/validation.py (1)
1154-1156: LGTM: None-safe fsdp_config access prevents AttributeErrorSwitching to a local fsdp_config = data.get("fsdp_config") or {} correctly guards against the case where fsdp_config exists but is set to None. This directly addresses the reported AttributeError.
| fsdp_config = data.get("fsdp_config") or {} | ||
| if fsdp_config and fsdp_config.get("fsdp_version"): | ||
| LOG.warning( | ||
| "Configuring `fsdp_version` in `fsdp_config` is deprecated. " | ||
| "Please configure `fsdp_version` as a top-level field." | ||
| ) | ||
| data["fsdp_version"] = fsdp_config.pop("fsdp_version") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid silently overriding top-level fsdp_version when both are set; handle conflicts and reduce log spam
Current code promotes the nested value unconditionally, which can overwrite an explicitly-set top-level fsdp_version without warning if both are present and differ. Also, switch to warning_once to avoid repeated logs across multiple validations.
Proposed update:
- If both top-level and nested fsdp_version exist and differ, keep the top-level and drop the nested with a warning about the conflict.
- Otherwise, promote nested to top-level and pop it from fsdp_config.
- Keep the None-safe pattern.
- fsdp_config = data.get("fsdp_config") or {}
- if fsdp_config and fsdp_config.get("fsdp_version"):
- LOG.warning(
- "Configuring `fsdp_version` in `fsdp_config` is deprecated. "
- "Please configure `fsdp_version` as a top-level field."
- )
- data["fsdp_version"] = fsdp_config.pop("fsdp_version")
+ fsdp_config = data.get("fsdp_config") or {}
+ nested_version = fsdp_config.get("fsdp_version")
+ if nested_version is not None:
+ existing = data.get("fsdp_version")
+ if existing is not None and str(existing) != str(nested_version):
+ LOG.warning(
+ "Conflicting `fsdp_version`: top-level=%s, fsdp_config=%s; "
+ "keeping top-level and dropping nested.",
+ existing,
+ nested_version,
+ )
+ fsdp_config.pop("fsdp_version", None)
+ else:
+ LOG.warning_once(
+ "Configuring `fsdp_version` in `fsdp_config` is deprecated. "
+ "Please configure `fsdp_version` as a top-level field."
+ )
+ data["fsdp_version"] = fsdp_config.pop("fsdp_version")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fsdp_config = data.get("fsdp_config") or {} | |
| if fsdp_config and fsdp_config.get("fsdp_version"): | |
| LOG.warning( | |
| "Configuring `fsdp_version` in `fsdp_config` is deprecated. " | |
| "Please configure `fsdp_version` as a top-level field." | |
| ) | |
| data["fsdp_version"] = fsdp_config.pop("fsdp_version") | |
| fsdp_config = data.get("fsdp_config") or {} | |
| nested_version = fsdp_config.get("fsdp_version") | |
| if nested_version is not None: | |
| existing = data.get("fsdp_version") | |
| if existing is not None and str(existing) != str(nested_version): | |
| LOG.warning( | |
| "Conflicting `fsdp_version`: top-level=%s, fsdp_config=%s; " | |
| "keeping top-level and dropping nested.", | |
| existing, | |
| nested_version, | |
| ) | |
| fsdp_config.pop("fsdp_version", None) | |
| else: | |
| LOG.warning_once( | |
| "Configuring `fsdp_version` in `fsdp_config` is deprecated. " | |
| "Please configure `fsdp_version` as a top-level field." | |
| ) | |
| data["fsdp_version"] = fsdp_config.pop("fsdp_version") |
🤖 Prompt for AI Agents
In src/axolotl/utils/schemas/validation.py around lines 820 to 826, the current
code unconditionally promotes a nested fsdp_version into the top-level which can
silently overwrite an explicit top-level value and spam logs; change the logic
to: read fsdp_config safely, if nested fsdp_version exists then check if data
already has a top-level fsdp_version — if both exist and differ, keep the
top-level value, pop the nested fsdp_config["fsdp_version"], and emit a single
warning_once describing the conflict and that the nested value was ignored;
otherwise (no top-level present) promote the nested value to
data["fsdp_version"], pop it from fsdp_config, and use warning_once for the
deprecation message when promoting.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
📖 Documentation Preview: https://689c6dce7dc401466a7e45bc--resonant-treacle-0fd729.netlify.app Deployed on Netlify from commit c77a96f |
3 participants
Description
Had a minor report in Discord https://discord.com/channels/1104757954588196865/1111279858136383509/1403385346288259225 where
fsdp_configwas None which would lead toThis PR fixes the check.
Reason for error:
fsdp_config=Noneinmerge_loraaxolotl/src/axolotl/cli/merge_lora.py
Line 75 in e0a2523
get(found the variable exists and returns that, even if it's NoneMotivation and Context
How has this been tested?
Screenshots (if appropriate)
Types of changes
Social Handles (Optional)
Summary by CodeRabbit
Bug Fixes
Refactor
Chores