Skip to content

feat: when validating OptimizationValidationMixin, fsdp_version shouldn't be flag as an warning#3718

Merged
winglian merged 1 commit into
axolotl-ai-cloud:mainfrom
SamuelLarkin:dev.sl/PR
Jun 9, 2026
Merged

feat: when validating OptimizationValidationMixin, fsdp_version shouldn't be flag as an warning#3718
winglian merged 1 commit into
axolotl-ai-cloud:mainfrom
SamuelLarkin:dev.sl/PR

Conversation

@SamuelLarkin

@SamuelLarkin SamuelLarkin commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

With the following config.yaml

fsdp_version: 2
fsdp_config:
  offload_params: false
  state_dict_type: FULL_STATE_DICT
  auto_wrap_policy: TRANSFORMER_BASED_WRAP
  transformer_layer_cls_to_wrap: LlamaDecoderLayer
  reshard_after_forward: true

We get the following WARNING

[2026-06-08 12:08:41,580] [WARNING] [axolotl.utils.schemas.validation] Configuring FSDP fields with the `fsdp_` prefix is deprecated. Please omit the `fsdp_` prefix from the any fields in `fsdp_config`.

Where

key == "fsdp_version"
fsdp_config={'offload_params': False, 'state_dict_type': 'FULL_STATE_DICT', 'auto_wrap_policy': 'TRANSFORMER_BASED_WRAP', 'transformer_layer_cls_to_wrap': 'LlamaDecoderLayer', 'reshard_after_forward': True, 'fsdp_version': 2}

fsdp_version is part of FSDPConfig

Summary by CodeRabbit

Bug Fixes

  • Resolved an issue where fsdp_version in FSDP configurations was incorrectly treated as deprecated and rewritten during configuration normalization.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR refines the FSDP configuration validation logic to exclude fsdp_version from the deprecated fsdp_*-prefixed field handling. A single-line change in the validation method now explicitly checks that a key is not fsdp_version before treating it as a deprecated field needing migration into the fsdp_config dictionary.

Changes

FSDP configuration validation refinement

Layer / File(s) Summary
FSDP version exclusion from deprecated key handling
src/axolotl/utils/schemas/validation.py
The check_fsdp_config_kwargs_prefix validation method now excludes fsdp_version from the deprecated fsdp_* prefix rewrite logic by adding a key != "fsdp_version" condition alongside the existing key.startswith("fsdp_") check.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • axolotl-ai-cloud/axolotl#3170: Both PRs modify FSDP config normalization behavior around fsdp_version to prevent it from being treated as a deprecated fsdp_* key.

Suggested reviewers

  • NanoCode012
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: preventing fsdp_version from being flagged as a deprecation warning during validation, which directly aligns with the file-level change in validation.py.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
src/axolotl/utils/schemas/validation.py (1)

1091-1092: 💤 Low value

Optional: Consider clarifying the warning message to reflect the fsdp_version exception.

The warning says "Please omit the fsdp_ prefix from the any fields in fsdp_config", but fsdp_version is now explicitly excluded from this requirement. For improved clarity, consider:

-                        "Configuring FSDP fields with the `fsdp_` prefix is deprecated. "
-                        "Please omit the `fsdp_` prefix from the any fields in `fsdp_config`."
+                        "Configuring FSDP fields with the `fsdp_` prefix is deprecated. "
+                        "Please omit the `fsdp_` prefix from fields in `fsdp_config` (except `fsdp_version`)."

This is a minor documentation improvement and not critical for functionality.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/axolotl/utils/schemas/validation.py` around lines 1091 - 1092, Update the
deprecation warning text in validation.py that mentions configuring FSDP fields
with the `fsdp_` prefix to explicitly note the exception for `fsdp_version`;
locate the message that currently reads "Configuring FSDP fields with the
`fsdp_` prefix is deprecated. Please omit the `fsdp_` prefix from the any fields
in `fsdp_config`." and revise it to call out that `fsdp_version` is excluded
(e.g., mention "except `fsdp_version`") so users know that `fsdp_version` may
still keep the prefix when using `fsdp_config`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/axolotl/utils/schemas/validation.py`:
- Around line 1091-1092: Update the deprecation warning text in validation.py
that mentions configuring FSDP fields with the `fsdp_` prefix to explicitly note
the exception for `fsdp_version`; locate the message that currently reads
"Configuring FSDP fields with the `fsdp_` prefix is deprecated. Please omit the
`fsdp_` prefix from the any fields in `fsdp_config`." and revise it to call out
that `fsdp_version` is excluded (e.g., mention "except `fsdp_version`") so users
know that `fsdp_version` may still keep the prefix when using `fsdp_config`.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a798cd0b-e99d-4456-80e9-9b1e84abab70

📥 Commits

Reviewing files that changed from the base of the PR and between b99ae09 and ed730d6.

📒 Files selected for processing (1)
  • src/axolotl/utils/schemas/validation.py

@winglian winglian left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

good catch. thanks

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@winglian winglian merged commit 77706d1 into axolotl-ai-cloud:main Jun 9, 2026
16 of 18 checks passed
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.

2 participants