[ci][deps] raydepsts: Improved error messages #58058
Conversation
Signed-off-by: elliot-barn <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the error messages in raydepsets, making them more informative and context-rich. The changes to include depset names, config file names, and underlying error details from subprocesses will greatly aid in debugging. The accompanying updates to the unit tests to verify these new error messages are also a great addition.
I've added a few minor suggestions to further improve code clarity, consistency, and remove some leftover debugging code. Overall, this is a solid improvement to the tool's usability.
| cmd = [self._uv_binary, "pip", cmd, *args] | ||
| click.echo(f"Executing command: {cmd}") | ||
| status = subprocess.run(cmd, cwd=self.workspace.dir, input=stdin) | ||
| click.echo(f"Executing command: {' '.join(cmd)}") |
There was a problem hiding this comment.
For logging the command, using shlex.join(cmd) is generally safer than ' '.join(cmd) as it correctly handles arguments that contain spaces or other special characters. This makes the logged command string easier to copy and paste into a shell for debugging.
| click.echo(f"Executing command: {' '.join(cmd)}") | |
| click.echo(f"Executing command: {shlex.join(cmd)}") |
| raise RuntimeError( | ||
| f"Failed to execute command: {' '.join(cmd)} with error: {status.stderr.decode('utf-8')}" | ||
| ) |
There was a problem hiding this comment.
Similarly here, using shlex.join(cmd) would be better for creating a reproducible command string in the error message. To avoid calling shlex.join twice, you could consider storing the result in a variable before the click.echo call on line 237.
| raise RuntimeError( | |
| f"Failed to execute command: {' '.join(cmd)} with error: {status.stderr.decode('utf-8')}" | |
| ) | |
| raise RuntimeError( | |
| f"Failed to execute command: {shlex.join(cmd)} with error: {status.stderr.decode('utf-8')}" | |
| ) |
| build_arg_sets=["invalid_build_arg_set"], | ||
| ) | ||
| workspace = Workspace(dir=tmpdir) | ||
| with unittest.TestCase().assertRaises(KeyError) as e: |
There was a problem hiding this comment.
Since this is a pytest test file, it would be more idiomatic to use pytest.raises instead of unittest.TestCase().assertRaises.
| with unittest.TestCase().assertRaises(KeyError) as e: | |
| with pytest.raises(KeyError) as e: |
| workspace = Workspace(dir=tmpdir) | ||
| with unittest.TestCase().assertRaises(KeyError) as e: | ||
| workspace.load_config(config_path=Path(tmpdir) / "test.depsets.yaml") | ||
| print(str(e.exception)) |
There was a problem hiding this comment.
This print statement appears to be for debugging and should be removed before merging.
| {f"pre_hooks: {depset.pre_hooks}" if depset.pre_hooks else ""} | ||
| {f"depsets: {depset.depsets}" if depset.depsets else ""} | ||
| {f"source_depset: {depset.source_depset}" if depset.source_depset else ""} | ||
| {f"config_name: {depset.config_name}" if depset.config_name else ""} |
There was a problem hiding this comment.
The config_name field is a required string argument in the Depset dataclass, so it should never be None or an empty string. This conditional check is redundant and can be removed for clarity.
| {f"config_name: {depset.config_name}" if depset.config_name else ""} | |
| config_name: {depset.config_name} |
improving error messages for raydepsets Updated unit tests to verify error messages Signed-off-by: elliot-barn <[email protected]> Signed-off-by: xgui <[email protected]>
improving error messages for raydepsets Updated unit tests to verify error messages Signed-off-by: elliot-barn <[email protected]>
improving error messages for raydepsets Updated unit tests to verify error messages Signed-off-by: elliot-barn <[email protected]> Signed-off-by: Aydin Abiar <[email protected]>
improving error messages for raydepsets Updated unit tests to verify error messages Signed-off-by: elliot-barn <[email protected]> Signed-off-by: Future-Outlier <[email protected]>
improving error messages for raydepsets
Updated unit tests to verify error messages