Conversation
Signed-off-by: abrar <abrar@anyscale.com>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new developer API to retrieve application URLs dynamically rather than hardcoding them. The key changes include:
- Replacing literal URLs in tests with dynamic calls to get_application_url(s).
- Adding get_application_urls and get_application_url API methods in python/ray/serve/api.py.
- Updating controller and constant definitions to support proxy URL selection and maintain consistency.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| python/ray/serve/tests/test_api.py | Updated test cases to use dynamic API calls for application URLs |
| python/ray/serve/api.py | Added new API methods to get application URLs with proper protocol support |
| python/ray/serve/_private/controller.py | Modified target group retrieval to support proxy and gRPC URL formation |
| python/ray/serve/_private/constants.py | Introduced a constant to control URL selection behavior |
| python/ray/serve/init.py | Updated exported API to include the new URL methods |
Comments suppressed due to low confidence (1)
python/ray/serve/api.py:1157
- Document the non-deterministic behavior of get_application_url when multiple URLs are available to ensure that users understand a random URL is returned, which is important for clarity in multi-target setups.
return random.choice(get_application_urls(protocol, app_name, proxy_ingress))
python/ray/serve/api.py
Outdated
| proxy_ingress: Whether to use the proxy ingress. | ||
|
|
||
| Returns: | ||
| The URL of the application. If there are multiple URLs, a random one is returned. |
There was a problem hiding this comment.
why would there ever be multiple?
There was a problem hiding this comment.
when location -> EveryNode, there will be one target per node.
python/ray/serve/api.py
Outdated
| Args: | ||
| protocol: The protocol to use for the application. | ||
| app_name: The name of the application. | ||
| proxy_ingress: Whether to use the proxy ingress. |
There was a problem hiding this comment.
"proxy ingress" is a highly confusing term because we call the deployment that handles HTTP the "ingress"
There was a problem hiding this comment.
How about use_proxy_targets? and change the env var to RAY_SERVE_USE_PROXY_TARGETS_TO_GET_APPLICATION_URLS
| targets=self.proxy_state_manager.get_targets(RequestProtocol.HTTP), | ||
| ) | ||
| ) | ||
| if is_grpc_enabled(self.get_grpc_config()): |
There was a problem hiding this comment.
@akyang-anyscale this fixes a bug, before we would include the grpc targets even if grpc was not enabled.
|
discussed offline with @edoakes and we decided to move the function to a test_utils until the API stabilizes |
Signed-off-by: abrar <abrar@anyscale.com>
Signed-off-by: abrar <abrar@anyscale.com>
Signed-off-by: abrar <abrar@anyscale.com>
Signed-off-by: abrar <abrar@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
Signed-off-by: abrar <abrar@anyscale.com>
Signed-off-by: abrar <abrar@anyscale.com> Signed-off-by: elliot-barn <elliot.barnwell@anyscale.com>
No description provided.