feat(rayapp): Anyscale CLI pr4 add workspace operations

[elliot-barn]

Add comprehensive workspace management methods:

• WorkspaceTestConfig struct for workspace configuration
• createEmptyWorkspace() - create new workspace with build ID conversion
• terminateWorkspace() - terminate a workspace by name
• copyTemplateToWorkspace() - push template files to workspace
• pushTemplateToWorkspace() - push local files to workspace
• runCmdInWorkspace() - execute commands in workspace
• startWorkspace() - start a stopped workspace
• getWorkspaceStatus() - get current workspace status
• waitForWorkspaceState() - wait for workspace to reach desired state

Also add compute config conversion functionality:

• isOldComputeConfigFormat() - detect old config format
• ConvertComputeConfig() - convert old to new format
• ConvertComputeConfigFile() - convert config files
• Auto-conversion in CreateComputeConfig()

This completes the Anyscale CLI wrapper with full workspace lifecycle management capabilities.

[gemini-code-assist]

Summary of Changes

Hello @elliot-barn, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the rayapp Anyscale CLI wrapper by integrating a complete set of workspace lifecycle management capabilities. It allows for programmatic interaction with Anyscale workspaces, from creation and termination to running commands and monitoring status. Additionally, it introduces robust compute configuration conversion, ensuring that older configuration formats are seamlessly updated to the latest standard, thereby improving usability and maintaining compatibility for users.

Highlights

• Workspace Management: Introduced a comprehensive suite of methods for full workspace lifecycle management, including creation, termination, file synchronization, command execution, starting, status retrieval, and state waiting.
• Compute Config Conversion: Added functionality to detect and automatically convert old Anyscale compute configuration formats to the new format during creation, ensuring backward compatibility and seamless transitions.
• New Data Structures: Defined WorkspaceTestConfig for managing workspace test parameters and specific structs (OldComputeConfig, NewComputeConfig, etc.) to facilitate compute config format conversion.
• Comprehensive Testing: Included new unit tests for all added workspace operations, covering various scenarios including success, failure, and specific edge cases like invalid build IDs.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces comprehensive workspace management capabilities to the Anyscale CLI wrapper, including creation, termination, and execution of commands within workspaces. It also adds a helpful utility to automatically convert legacy compute configuration files to the new format. The code is well-structured and includes a good set of unit tests for the new functionality. My review includes a few suggestions to improve error handling, consistency, and correct a potential bug in command execution.

[gemini-code-assist]

The cmd string is passed as a single argument to the anyscale CLI. This will likely fail if the command contains arguments (e.g., ls -l). The command string should be split into its components. Using strings.Fields() is a simple way to achieve this for commands without quoted arguments. For more complex use cases, a shell-aware parser might be needed, but strings.Fields should cover many common scenarios.

func (ac *AnyscaleCLI) runCmdInWorkspace(config *WorkspaceTestConfig, cmd string) error {
	args := []string{"workspace_v2", "run_command", "--name", config.workspaceName}
	args = append(args, strings.Fields(cmd)...)
	output, err := ac.runAnyscaleCLI(args)
	if err != nil {
		return fmt.Errorf("run command in workspace failed: %w", err)
	}
	fmt.Println("run command in workspace output:\n", output)
	return nil
}

[gitar-bot]

I've addressed all 4 issues identified in the code review:

1. Fixed temp file race condition (Important)

• Moved os.Remove() to execute after runAnyscaleCLI() completes instead of using defer
• This ensures the CLI command finishes reading the file before cleanup

2. Added nil pointer checks in createEmptyWorkspace() (Important)

• Added validation for config.template and config.template.ClusterEnv before accessing BuildID
• Returns clear error messages if nil

3. Added nil pointer check in copyTemplateToWorkspace() (Minor)

• Added validation for config.template before accessing Dir
• Returns clear error message if nil

4. Added comprehensive unit tests (Minor)

• TestIsOldComputeConfigFormat: 5 test cases covering old/new formats, file errors, invalid YAML
• TestConvertComputeConfig: 3 test cases for successful conversion, file errors, invalid YAML
• TestConvertComputeConfigFile: 3 test cases for file output, stdout output, and error handling
• All new tests pass ✅

Bonus: Fixed go.mod to use valid Go version format (1.23 instead of 1.25.2)

[gitar-bot]

I've re-applied all 4 fixes from the code review after the Gemini suggestions were merged:

1. Fixed temp file race condition (Important) ✅

• Moved os.Remove() to execute after runAnyscaleCLI() completes
• Preserved Gemini's improvement to check tmpFile.Close() error
• Now ensures CLI command finishes reading the file before cleanup

2. Added nil pointer checks in createEmptyWorkspace() (Important) ✅

• Validates config.template and config.template.ClusterEnv before accessing BuildID
• Returns clear error messages if nil

3. Added nil pointer check in copyTemplateToWorkspace() (Minor) ✅

• Validates config.template before accessing Dir
• Returns clear error message if nil

4. Added comprehensive unit tests (Minor) ✅

• TestIsOldComputeConfigFormat: 5 test cases
• TestConvertComputeConfig: 3 test cases
• TestConvertComputeConfigFile: 3 test cases
• All 11 new tests pass ✅

Bonus: Fixed go.mod version (1.23)

[gitar-bot]

All 4 code review findings have been addressed:

1. Temp file race condition ✅

• Moved os.Remove() to execute after runAnyscaleCLI() completes
• Prevents file deletion before CLI command finishes reading
• Maintains Gemini's error checking for tmpFile.Close()

2. Nil pointer checks in createEmptyWorkspace() ✅

• Added validation for config.template and config.template.ClusterEnv
• Returns clear error messages if nil

3. Nil pointer check in copyTemplateToWorkspace() ✅

• Added validation for config.template
• Returns clear error message if nil

4. Missing test coverage ✅

• Added 11 comprehensive unit tests:
  • TestIsOldComputeConfigFormat: 5 test cases
  • TestConvertComputeConfig: 3 test cases
  • TestConvertComputeConfigFile: 3 test cases
• All tests pass ✅

Bonus: Fixed go.mod version (1.23)

[gitar-bot]

Code Review ✅ Approved 4 resolved / 4 findings

Well-structured implementation of workspace operations and compute config conversion. The code demonstrates good error handling with nil checks, proper temp file cleanup, and comprehensive test coverage. No significant issues found.

✅ 4 resolved

✅ Bug: Temp file may be deleted before CLI command reads it

📄 rayapp/anyscale_cli.go:90
In CreateComputeConfig(), a temporary file is created for the converted config and defer os.Remove(tmpFile.Name()) is called. The CLI command runAnyscaleCLI() is then executed using this temp file path.

The issue is that runAnyscaleCLI() likely runs the CLI command asynchronously or the CLI itself may read the file after returning control. However, since defer executes at function return, the temp file will be deleted immediately after the function returns.

Looking at the code flow:

tmpFile, err := os.CreateTemp("", "compute-config-*.yaml")
defer os.Remove(tmpFile.Name())  // Will delete immediately on function return
// ... write to file ...
output, err := ac.runAnyscaleCLI(args)  // CLI reads the file
// Function returns, defer executes, file deleted

If runAnyscaleCLI returns successfully before or during the CLI's file read operation completing, this could cause intermittent failures.

Suggested fix: Consider either:

1. Not using defer and explicitly removing the file after confirming the operation completed
2. Keeping the temp file around longer (e.g., using a cleanup function returned to the caller)
3. If runAnyscaleCLI is synchronous and blocks until completion, add a comment documenting this assumption

✅ Edge Case: Potential nil pointer dereference in createEmptyWorkspace

📄 rayapp/anyscale_cli.go:116-117
The createEmptyWorkspace method accesses config.template.ClusterEnv.BuildID without checking for nil pointers in the chain:

func (ac *AnyscaleCLI) createEmptyWorkspace(config *WorkspaceTestConfig) error {
    args := []string{"workspace_v2", "create"}
    imageURI, rayVersion, err := convertBuildIdToImageURI(config.template.ClusterEnv.BuildID)
    // ...
}

If config.template or config.template.ClusterEnv is nil, this will panic. While these are internal (unexported) methods, defensive nil checks would make the code more robust.

Suggested fix: Add nil checks before dereferencing:

if config == nil || config.template == nil || config.template.ClusterEnv == nil {
    return fmt.Errorf("invalid config: template and ClusterEnv must not be nil")
}

✅ Edge Case: Potential nil pointer in copyTemplateToWorkspace

📄 rayapp/anyscale_cli.go:147
The copyTemplateToWorkspace method accesses config.template.Dir without checking if config.template is nil:

func (ac *AnyscaleCLI) copyTemplateToWorkspace(config *WorkspaceTestConfig) error {
    output, err := ac.runAnyscaleCLI([]string{"workspace_v2", "push", "--name", config.workspaceName, "--local-dir", config.template.Dir})
    // ...
}

While this is an internal method, it would benefit from a nil check to provide a clearer error message if misused.

Suggested fix: Add a nil check or document the requirement that config.template must be non-nil.

✅ Quality: Missing tests for ConvertComputeConfig functions

📄 rayapp/anyscale_cli.go:229 📄 rayapp/anyscale_cli.go:259
The PR adds several exported functions for compute config conversion:

• ConvertComputeConfig() (exported)
• ConvertComputeConfigFile() (exported)
• isOldComputeConfigFormat() (unexported)

However, the test file doesn't include any tests for these functions. Since ConvertComputeConfig and ConvertComputeConfigFile are exported (public API), they should have unit tests covering:

1. Successful conversion from old to new format
2. Invalid YAML handling
3. File not found errors
4. ConvertComputeConfigFile with empty output path (stdout case)

Suggested fix: Add test functions like:

func TestConvertComputeConfig(t *testing.T) { ... }
func TestConvertComputeConfigFile(t *testing.T) { ... }
func TestIsOldComputeConfigFormat(t *testing.T) { ... }

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}