[router] remove worker url requirement (#13172)
This commit is contained in:
@@ -677,12 +677,9 @@ class RouterArgs:
|
|||||||
def _validate_router_args(self):
|
def _validate_router_args(self):
|
||||||
# Validate configuration based on mode
|
# Validate configuration based on mode
|
||||||
if self.pd_disaggregation:
|
if self.pd_disaggregation:
|
||||||
# Validate PD configuration - skip URL requirements if using service discovery
|
# Allow empty URLs even without service discovery to support dynamic worker addition
|
||||||
if not self.service_discovery:
|
# URLs will be validated separately if provided
|
||||||
if not self.prefill_urls:
|
pass
|
||||||
raise ValueError("PD disaggregation mode requires --prefill")
|
|
||||||
if not self.decode_urls:
|
|
||||||
raise ValueError("PD disaggregation mode requires --decode")
|
|
||||||
|
|
||||||
# Warn about policy usage in PD mode
|
# Warn about policy usage in PD mode
|
||||||
if self.prefill_policy and self.decode_policy and self.policy:
|
if self.prefill_policy and self.decode_policy and self.policy:
|
||||||
|
|||||||
@@ -53,8 +53,8 @@ class TestRouterConfigValidation:
|
|||||||
assert args.decode_urls == ["http://decode1:8001", "http://decode2:8001"]
|
assert args.decode_urls == ["http://decode1:8001", "http://decode2:8001"]
|
||||||
assert args.policy == "cache_aware"
|
assert args.policy == "cache_aware"
|
||||||
|
|
||||||
def test_pd_config_without_urls_raises_error(self):
|
def test_pd_config_without_urls_allowed(self):
|
||||||
"""Test that PD mode without URLs raises validation error."""
|
"""Test that PD mode without URLs is now allowed (URLs are optional)."""
|
||||||
args = RouterArgs(
|
args = RouterArgs(
|
||||||
pd_disaggregation=True,
|
pd_disaggregation=True,
|
||||||
prefill_urls=[],
|
prefill_urls=[],
|
||||||
@@ -62,11 +62,14 @@ class TestRouterConfigValidation:
|
|||||||
service_discovery=False,
|
service_discovery=False,
|
||||||
)
|
)
|
||||||
|
|
||||||
# This should raise an error when trying to launch
|
# Should not raise validation error - URLs are now optional
|
||||||
with pytest.raises(
|
with patch("sglang_router.launch_router.Router") as router_mod:
|
||||||
ValueError, match="PD disaggregation mode requires --prefill"
|
mock_router_instance = MagicMock()
|
||||||
):
|
router_mod.from_args = MagicMock(return_value=mock_router_instance)
|
||||||
|
|
||||||
|
# This should succeed without raising an error
|
||||||
launch_router(args)
|
launch_router(args)
|
||||||
|
router_mod.from_args.assert_called_once()
|
||||||
|
|
||||||
def test_pd_config_with_service_discovery_allows_empty_urls(self):
|
def test_pd_config_with_service_discovery_allows_empty_urls(self):
|
||||||
"""Test that PD mode with service discovery allows empty URLs."""
|
"""Test that PD mode with service discovery allows empty URLs."""
|
||||||
|
|||||||
@@ -464,7 +464,7 @@ class TestStartupValidation:
|
|||||||
|
|
||||||
def test_pd_mode_validation_during_startup(self):
|
def test_pd_mode_validation_during_startup(self):
|
||||||
"""Test PD mode validation during startup."""
|
"""Test PD mode validation during startup."""
|
||||||
# PD mode without URLs should fail
|
# PD mode without URLs is now allowed (URLs are optional)
|
||||||
args = RouterArgs(
|
args = RouterArgs(
|
||||||
pd_disaggregation=True,
|
pd_disaggregation=True,
|
||||||
prefill_urls=[],
|
prefill_urls=[],
|
||||||
@@ -472,10 +472,14 @@ class TestStartupValidation:
|
|||||||
service_discovery=False,
|
service_discovery=False,
|
||||||
)
|
)
|
||||||
|
|
||||||
with pytest.raises(
|
# Should not raise validation error - URLs are now optional
|
||||||
ValueError, match="PD disaggregation mode requires --prefill"
|
with patch("sglang_router.launch_router.Router") as router_mod:
|
||||||
):
|
mock_router_instance = MagicMock()
|
||||||
|
router_mod.from_args = MagicMock(return_value=mock_router_instance)
|
||||||
|
|
||||||
|
# This should succeed without raising an error
|
||||||
launch_router(args)
|
launch_router(args)
|
||||||
|
router_mod.from_args.assert_called_once()
|
||||||
|
|
||||||
def test_pd_mode_with_service_discovery_validation(self):
|
def test_pd_mode_with_service_discovery_validation(self):
|
||||||
"""Test PD mode with service discovery validation during startup."""
|
"""Test PD mode with service discovery validation during startup."""
|
||||||
|
|||||||
@@ -400,9 +400,9 @@ class TestConfigurationValidation:
|
|||||||
class TestLaunchValidation:
|
class TestLaunchValidation:
|
||||||
"""Test launch-time validation logic."""
|
"""Test launch-time validation logic."""
|
||||||
|
|
||||||
def test_pd_mode_requires_urls(self):
|
def test_pd_mode_allows_empty_urls(self):
|
||||||
"""Test that PD mode requires prefill and decode URLs."""
|
"""Test that PD mode now allows empty URLs (URLs are optional)."""
|
||||||
# PD mode without URLs should fail
|
# PD mode without URLs is now allowed
|
||||||
args = RouterArgs(
|
args = RouterArgs(
|
||||||
pd_disaggregation=True,
|
pd_disaggregation=True,
|
||||||
prefill_urls=[],
|
prefill_urls=[],
|
||||||
@@ -410,10 +410,14 @@ class TestLaunchValidation:
|
|||||||
service_discovery=False,
|
service_discovery=False,
|
||||||
)
|
)
|
||||||
|
|
||||||
with pytest.raises(
|
# Should not raise validation error - URLs are now optional
|
||||||
ValueError, match="PD disaggregation mode requires --prefill"
|
with patch("sglang_router.launch_router.Router") as router_mod:
|
||||||
):
|
mock_router_instance = MagicMock()
|
||||||
|
router_mod.from_args = MagicMock(return_value=mock_router_instance)
|
||||||
|
|
||||||
|
# This should succeed without raising an error
|
||||||
launch_router(args)
|
launch_router(args)
|
||||||
|
router_mod.from_args.assert_called_once()
|
||||||
|
|
||||||
def test_pd_mode_with_service_discovery_allows_empty_urls(self):
|
def test_pd_mode_with_service_discovery_allows_empty_urls(self):
|
||||||
"""Test that PD mode with service discovery allows empty URLs."""
|
"""Test that PD mode with service discovery allows empty URLs."""
|
||||||
|
|||||||
@@ -6,9 +6,7 @@ pub struct ConfigValidator;
|
|||||||
|
|
||||||
impl ConfigValidator {
|
impl ConfigValidator {
|
||||||
pub fn validate(config: &RouterConfig) -> ConfigResult<()> {
|
pub fn validate(config: &RouterConfig) -> ConfigResult<()> {
|
||||||
let has_service_discovery = config.discovery.as_ref().is_some_and(|d| d.enabled);
|
Self::validate_mode(&config.mode)?;
|
||||||
|
|
||||||
Self::validate_mode(&config.mode, has_service_discovery)?;
|
|
||||||
Self::validate_policy(&config.policy)?;
|
Self::validate_policy(&config.policy)?;
|
||||||
Self::validate_server_settings(config)?;
|
Self::validate_server_settings(config)?;
|
||||||
|
|
||||||
@@ -89,7 +87,7 @@ impl ConfigValidator {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
fn validate_mode(mode: &RoutingMode, has_service_discovery: bool) -> ConfigResult<()> {
|
fn validate_mode(mode: &RoutingMode) -> ConfigResult<()> {
|
||||||
match mode {
|
match mode {
|
||||||
RoutingMode::Regular { worker_urls } => {
|
RoutingMode::Regular { worker_urls } => {
|
||||||
if !worker_urls.is_empty() {
|
if !worker_urls.is_empty() {
|
||||||
@@ -103,19 +101,8 @@ impl ConfigValidator {
|
|||||||
prefill_policy,
|
prefill_policy,
|
||||||
decode_policy,
|
decode_policy,
|
||||||
} => {
|
} => {
|
||||||
if !has_service_discovery {
|
// Allow empty URLs even without service discovery to support dynamic worker addition
|
||||||
if prefill_urls.is_empty() {
|
// URLs will be validated if provided
|
||||||
return Err(ConfigError::ValidationFailed {
|
|
||||||
reason: "PD mode requires at least one prefill worker URL".to_string(),
|
|
||||||
});
|
|
||||||
}
|
|
||||||
if decode_urls.is_empty() {
|
|
||||||
return Err(ConfigError::ValidationFailed {
|
|
||||||
reason: "PD mode requires at least one decode worker URL".to_string(),
|
|
||||||
});
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
if !prefill_urls.is_empty() {
|
if !prefill_urls.is_empty() {
|
||||||
let prefill_url_strings: Vec<String> =
|
let prefill_url_strings: Vec<String> =
|
||||||
prefill_urls.iter().map(|(url, _)| url.clone()).collect();
|
prefill_urls.iter().map(|(url, _)| url.clone()).collect();
|
||||||
@@ -145,14 +132,13 @@ impl ConfigValidator {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
RoutingMode::OpenAI { worker_urls } => {
|
RoutingMode::OpenAI { worker_urls } => {
|
||||||
if worker_urls.is_empty() {
|
// Allow empty URLs to support dynamic worker addition
|
||||||
return Err(ConfigError::ValidationFailed {
|
// URLs will be validated if provided
|
||||||
reason: "OpenAI mode requires at least one --worker-urls entry".to_string(),
|
if !worker_urls.is_empty() {
|
||||||
});
|
|
||||||
}
|
|
||||||
Self::validate_urls(worker_urls)?;
|
Self::validate_urls(worker_urls)?;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -888,6 +874,45 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_validate_empty_urls_allowed_without_service_discovery() {
|
||||||
|
// Test that empty URLs are now allowed in PD mode
|
||||||
|
let config = RouterConfig::new(
|
||||||
|
RoutingMode::PrefillDecode {
|
||||||
|
prefill_urls: vec![],
|
||||||
|
decode_urls: vec![],
|
||||||
|
prefill_policy: None,
|
||||||
|
decode_policy: None,
|
||||||
|
},
|
||||||
|
PolicyConfig::Random,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should pass validation even with empty URLs
|
||||||
|
assert!(ConfigValidator::validate(&config).is_ok());
|
||||||
|
|
||||||
|
// Test that empty URLs are allowed in Regular mode
|
||||||
|
let config = RouterConfig::new(
|
||||||
|
RoutingMode::Regular {
|
||||||
|
worker_urls: vec![],
|
||||||
|
},
|
||||||
|
PolicyConfig::Random,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should pass validation even with empty URLs
|
||||||
|
assert!(ConfigValidator::validate(&config).is_ok());
|
||||||
|
|
||||||
|
// Test that empty URLs are allowed in OpenAI mode
|
||||||
|
let config = RouterConfig::new(
|
||||||
|
RoutingMode::OpenAI {
|
||||||
|
worker_urls: vec![],
|
||||||
|
},
|
||||||
|
PolicyConfig::Random,
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should pass validation even with empty URLs
|
||||||
|
assert!(ConfigValidator::validate(&config).is_ok());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_validate_grpc_requires_tokenizer() {
|
fn test_validate_grpc_requires_tokenizer() {
|
||||||
let mut config = RouterConfig::new(
|
let mut config = RouterConfig::new(
|
||||||
|
|||||||
+2
-12
@@ -478,12 +478,7 @@ impl CliArgs {
|
|||||||
} else if self.pd_disaggregation {
|
} else if self.pd_disaggregation {
|
||||||
let decode_urls = self.decode.clone();
|
let decode_urls = self.decode.clone();
|
||||||
|
|
||||||
if !self.service_discovery && (prefill_urls.is_empty() || decode_urls.is_empty()) {
|
// Allow empty URLs to support dynamic worker addition
|
||||||
return Err(ConfigError::ValidationFailed {
|
|
||||||
reason: "PD disaggregation mode requires --prefill and --decode URLs when not using service discovery".to_string(),
|
|
||||||
});
|
|
||||||
}
|
|
||||||
|
|
||||||
RoutingMode::PrefillDecode {
|
RoutingMode::PrefillDecode {
|
||||||
prefill_urls,
|
prefill_urls,
|
||||||
decode_urls,
|
decode_urls,
|
||||||
@@ -491,12 +486,7 @@ impl CliArgs {
|
|||||||
decode_policy: self.decode_policy.as_ref().map(|p| self.parse_policy(p)),
|
decode_policy: self.decode_policy.as_ref().map(|p| self.parse_policy(p)),
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
if !self.service_discovery && self.worker_urls.is_empty() {
|
// Allow empty URLs to support dynamic worker addition
|
||||||
return Err(ConfigError::ValidationFailed {
|
|
||||||
reason: "Regular mode requires --worker-urls when not using service discovery"
|
|
||||||
.to_string(),
|
|
||||||
});
|
|
||||||
}
|
|
||||||
RoutingMode::Regular {
|
RoutingMode::Regular {
|
||||||
worker_urls: self.worker_urls.clone(),
|
worker_urls: self.worker_urls.clone(),
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user