[v0.5.10][7][FSDP] move FSDP to experimental and disable by default#961
[v0.5.10][7][FSDP] move FSDP to experimental and disable by default#961yueming-yuan wants to merge 6 commits intofix/fsdp-weight-sync-version-mismatchfrom
Conversation
- Move miles/backends/fsdp_utils/ to miles/backends/experimental/fsdp_utils/ - Add assertion to block --train-backend fsdp with deprecation message - Remove FSDP tests from CI jobs except run-ci-fsdp and run-ci-image - Add [FSDP] prefix to FSDP test names in CI matrix
There was a problem hiding this comment.
Code Review
This pull request moves the FSDP backend to the experimental directory and adds a runtime check to warn users about its current maintenance status. The reviewer correctly identified that using an assertion for this validation is unsafe because it can be disabled with the -O flag, and suggested replacing it with a ValueError to ensure the check is always enforced.
miles/utils/arguments.py
Outdated
| assert backend != "fsdp", ( | ||
| "The FSDP backend has known issues with SGLang v0.5.10 and is not actively maintained in the current version. " | ||
| "It has been moved to miles.backends.experimental. " | ||
| "Contributions are welcome if you are interested in improving it." | ||
| ) |
There was a problem hiding this comment.
Using assert for runtime validation of user-provided arguments is generally discouraged in Python. Assertions can be globally disabled using the -O optimization flag, which would bypass this safety check. For critical validation intended to block a feature or provide a user-facing error message, it is better to raise an exception like ValueError to ensure the check is always performed.
| assert backend != "fsdp", ( | |
| "The FSDP backend has known issues with SGLang v0.5.10 and is not actively maintained in the current version. " | |
| "It has been moved to miles.backends.experimental. " | |
| "Contributions are welcome if you are interested in improving it." | |
| ) | |
| if backend == "fsdp": | |
| raise ValueError( | |
| "The FSDP backend has known issues with SGLang v0.5.10 and is not actively maintained in the current version. " | |
| "It has been moved to miles.backends.experimental. " | |
| "Contributions are welcome if you are interested in improving it." | |
| ) |
…sdp-to-experimental
…sdp-to-experimental
maocheng23
left a comment
There was a problem hiding this comment.
Merge after resolve conflict please
ci-sglang-pr: sglang-miles-v0.5.10
Summary
miles/backends/fsdp_utils/tomiles/backends/experimental/fsdp_utils/and update all import paths--train-backend fsdpwith deprecation messagerun-ci-fsdpandrun-ci-image[FSDP]prefix to all FSDP test names in CI matrix