finishing commit for issue #52113 #53964
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the serve config CLI command to handle cases where no configuration has been deployed and adds a corresponding unit test.
- Prints a warning message when no configs are available for all apps or a specific app.
- Refactors
config()inscripts.pyto first collect configs into a list and branch on its emptiness. - Introduces
test_serve_config_no_config_prints_warningintest_cli.pyto verify the new warning.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| python/ray/serve/scripts.py | Refactored config() to print a warning if no configs |
| python/ray/serve/tests/test_cli.py | Added test verifying warning when no serve config exists |
Comments suppressed due to low confidence (1)
| # Fetch a specific app config by name. | ||
| else: | ||
| app = serve_details.applications.get(name) | ||
| if app is None or app.deployed_app_config is None: | ||
| print(f'No config has been deployed for application "{name}".') | ||
| else: | ||
| config = app.deployed_app_config.dict(exclude_unset=True) | ||
| print(yaml.safe_dump(config, sort_keys=False), end="") | ||
|
|
||
| print(yaml.safe_dump(config, sort_keys=False), end="" if config else "No config was provided during `serve run`. Nothing to display.") |
There was a problem hiding this comment.
Combining the YAML dump and warning via the end parameter can mix the two outputs. It’s clearer to branch: if config is nonempty, print the dump; otherwise, print only the warning.
| print(yaml.safe_dump(config, sort_keys=False), end="" if config else "No config was provided during `serve run`. Nothing to display.") | |
| if config: | |
| print(yaml.safe_dump(config, sort_keys=False)) | |
| else: | |
| print("No config was provided during `serve run`. Nothing to display.") |
Copilot uses AI. Check for mistakes.
| with pytest.raises(subprocess.CalledProcessError) as e: | ||
| subprocess.check_output(["serve", "config"], stderr=subprocess.STDOUT) | ||
| output = e.value.output.decode("utf-8") |
There was a problem hiding this comment.
The test expects a non-zero exit (exception) but the CLI now prints a warning and exits 0. Either change the CLI to exit with an error code or update the test to capture and assert on stdout instead of an exception.
| with pytest.raises(subprocess.CalledProcessError) as e: | |
| subprocess.check_output(["serve", "config"], stderr=subprocess.STDOUT) | |
| output = e.value.output.decode("utf-8") | |
| output = subprocess.check_output(["serve", "config"], stderr=subprocess.STDOUT).decode("utf-8") |
Copilot uses AI. Check for mistakes.
| subprocess.check_output(["serve", "config"], stderr=subprocess.STDOUT) | ||
| output = e.value.output.decode("utf-8") | ||
| assert "No config was provided during `serve run`. Nothing to display." in output | ||
| return True |
There was a problem hiding this comment.
[nitpick] A return value in a pytest test function is unnecessary. You can remove return True to keep the test concise.
| return True | |
Copilot uses AI. Check for mistakes.
Why are these changes needed?
Related issue number
Closes #52113
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.