docs #8394
docs #8394
Conversation
|
/build-docs |
|
✅ ReadTheDocs build triggered for branch The documentation will be available at: https://docs.skypilot.co/en/external-link-docs/ |
Summary of ChangesHello @lloyd-brown, I'm Gemini Code Assist1! 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 delivers a significant enhancement to managed jobs by introducing a 'Custom Links' feature. This allows users to enrich their job monitoring experience by providing direct, configurable links to relevant external resources such as experiment trackers, custom dashboards, or cloud provider consoles. The implementation covers various methods for defining these links, from static configuration in YAML to dynamic updates during job execution, and integrates them seamlessly into the SkyPilot dashboard for improved observability. Highlights
🧠 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 AssistThe 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
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 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
|
![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 introduces a valuable "custom links" feature for managed jobs, allowing users to associate URLs with their jobs for display in the dashboard. The implementation is comprehensive, covering the backend, CLI, and frontend dashboard, and is accompanied by thorough documentation and tests. My review focuses on a few opportunities to improve code maintainability by refactoring duplicated logic and removing a redundant operation. Overall, this is a well-executed feature addition.
| SETUP_SKY_DIRS_COMMANDS = ( | ||
| f'mkdir -p ~/sky_workdir && ' | ||
| f'mkdir -p ~/.sky/sky_app && ' | ||
| f'mkdir -p {SKY_RUNTIME_DIR}/.sky && ' | ||
| # Create empty links file for runtime link syncing | ||
| 'touch ~/.sky/links;') |
There was a problem hiding this comment.
The addition of touch ~/.sky/links to SETUP_SKY_DIRS_COMMANDS appears to be redundant. The logic to create this file for managed jobs is already handled in sky/backends/cloud_vm_ray_backend.py, where it's correctly scoped to only managed jobs. This change will create the file on all clusters provisioned by SkyPilot, which is not the intended scope for this feature. Please remove the line touch ~/.sky/links; to avoid redundancy and potential confusion.
SETUP_SKY_DIRS_COMMANDS = (
f'mkdir -p ~/sky_workdir && '
f'mkdir -p ~/.sky/sky_app && '
f'mkdir -p {SKY_RUNTIME_DIR}/.sky;')| # For managed jobs, prepend links file creation to setup command | ||
| setup_cmd = self._setup_cmd | ||
| if task.managed_job_dag is not None: | ||
| links_path = constants.SKYPILOT_LINKS_PATH | ||
| create_links_file_cmd = ( | ||
| f'mkdir -p $(dirname {links_path}) && touch {links_path}') | ||
| if setup_cmd is not None: | ||
| setup_cmd = f'{create_links_file_cmd} && {setup_cmd}' | ||
| else: | ||
| setup_cmd = create_links_file_cmd |
There was a problem hiding this comment.
This logic for prepending the links file creation command is duplicated in _execute_task_n_nodes. To improve maintainability and avoid code duplication, consider refactoring this block into a helper method. This method could take self._setup_cmd and task as arguments and return the modified setup_cmd.
| for k, v in env_vars.items(): | ||
| # Expand paths with ~ or $HOME for env vars that are file paths | ||
| if k == constants.SKYPILOT_LINKS_ENV_VAR: | ||
| sky_env_vars_dict_str.append( | ||
| f'sky_env_vars_dict[{k!r}] = os.path.expanduser({v!r})') | ||
| else: | ||
| sky_env_vars_dict_str.append( | ||
| f'sky_env_vars_dict[{k!r}] = {v!r}') |
There was a problem hiding this comment.
This logic for processing environment variables, especially the special handling for SKYPILOT_LINKS_ENV_VAR, is duplicated in the add_task method. To reduce code duplication and improve maintainability, this logic could be extracted into a private helper method that both _add_ray_task and add_task can call.
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)