Refactor DSpark disagg - #645
zhangxiaolei123456 merged 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the disaggregation metadata configuration and hidden bootstrap plan resolution by extracting the logic into a new module, 'dspark_disaggregation.py', and renaming several 'DSpark' prefixed classes to 'PD' equivalents. The review feedback suggests improving robustness in the new module by converting string enum values to lowercase before comparison and using 'or []' or 'or 0' instead of '.get(..., [])' or '.get(..., 0)' to safely handle cases where dictionary values might be explicitly set to 'None'.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| mode_value = getattr(disaggregation_mode, "value", disaggregation_mode) | ||
| if mode_value not in ("decode", "prefill"): | ||
| return DSparkDisaggMetadataConfig() |
There was a problem hiding this comment.
To ensure robustness against case-sensitivity issues, it is recommended to convert mode_value to lowercase before checking if it is in the allowed set of modes. This prevents potential runtime issues if the enum values or strings are passed in uppercase or mixed-case.
mode_value = getattr(disaggregation_mode, "value", disaggregation_mode)
if isinstance(mode_value, str):
mode_value = mode_value.lower()
if mode_value not in ("decode", "prefill"):
return DSparkDisaggMetadataConfig()| backend_value = getattr(transfer_backend, "value", transfer_backend) | ||
| if backend_value not in ("mooncake", "fake"): | ||
| raise NotImplementedError( | ||
| "DSpark PD hidden transfer is implemented only for Mooncake/Fake " | ||
| f"backends, got {backend_value}." | ||
| ) |
There was a problem hiding this comment.
Similarly, backend_value should be converted to lowercase to handle potential case mismatches robustly.
| backend_value = getattr(transfer_backend, "value", transfer_backend) | |
| if backend_value not in ("mooncake", "fake"): | |
| raise NotImplementedError( | |
| "DSpark PD hidden transfer is implemented only for Mooncake/Fake " | |
| f"backends, got {backend_value}." | |
| ) | |
| backend_value = getattr(transfer_backend, "value", transfer_backend) | |
| if isinstance(backend_value, str): | |
| backend_value = backend_value.lower() | |
| if backend_value not in ("mooncake", "fake"): | |
| raise NotImplementedError( | |
| "DSpark PD hidden transfer is implemented only for Mooncake/Fake " | |
| f"backends, got {backend_value}." | |
| ) |
| local_layer_ids = ( | ||
| [int(x) for x in local_pp_slice.get("layer_ids", [])] | ||
| if local_pp_slice | ||
| else ( | ||
| [] | ||
| if pp_slices | ||
| else [int(x) for x in metadata.get("target_layer_ids", [])] | ||
| ) | ||
| ) | ||
| local_slice_len = ( | ||
| int(local_pp_slice.get("slice_len", 0)) | ||
| if local_pp_slice | ||
| else len(local_layer_ids) * int(model_config.hidden_size) | ||
| ) |
There was a problem hiding this comment.
Using .get(..., []) or .get(..., 0) can return None if the key exists in the dictionary but its value is explicitly set to None (e.g., from a parsed JSON null). To prevent a TypeError during iteration or type conversion, use or [] and or 0 instead.
| local_layer_ids = ( | |
| [int(x) for x in local_pp_slice.get("layer_ids", [])] | |
| if local_pp_slice | |
| else ( | |
| [] | |
| if pp_slices | |
| else [int(x) for x in metadata.get("target_layer_ids", [])] | |
| ) | |
| ) | |
| local_slice_len = ( | |
| int(local_pp_slice.get("slice_len", 0)) | |
| if local_pp_slice | |
| else len(local_layer_ids) * int(model_config.hidden_size) | |
| ) | |
| local_layer_ids = ( | |
| [int(x) for x in (local_pp_slice.get("layer_ids") or [])] | |
| if local_pp_slice | |
| else ( | |
| [] | |
| if pp_slices | |
| else [int(x) for x in (metadata.get("target_layer_ids") or [])] | |
| ) | |
| ) | |
| local_slice_len = ( | |
| int(local_pp_slice.get("slice_len") or 0) | |
| if local_pp_slice | |
| else len(local_layer_ids) * int(model_config.hidden_size) | |
| ) |
| dst_indices = [ | ||
| int(x) | ||
| for x in ( | ||
| local_pp_slice.get("dst_indices", []) | ||
| if local_pp_slice | ||
| else metadata.get("dst_indices", []) | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Similarly, use or [] instead of .get(..., []) to safely handle cases where dst_indices might be explicitly set to None in the metadata.
| dst_indices = [ | |
| int(x) | |
| for x in ( | |
| local_pp_slice.get("dst_indices", []) | |
| if local_pp_slice | |
| else metadata.get("dst_indices", []) | |
| ) | |
| ] | |
| dst_indices = [ | |
| int(x) | |
| for x in ( | |
| local_pp_slice.get("dst_indices") or [] | |
| if local_pp_slice | |
| else metadata.get("dst_indices") or [] | |
| ) | |
| ] |
035e308 to
10e6a6a
Compare
10e6a6a to
f65f7ad
Compare
28b0747
into
deepseev_v4_dpsark_pd_dev
Motivation
Modifications
Accuracy Tests
Speed Tests and Profiling
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ciCI States
Latest PR Test (Base): ❌ Run #29752805296
Latest PR Test (Extra): ❌ Run #29752805120