example commit#1392
Conversation
|
/gcbrun |
![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 updates the GPU initialization action documentation to emphasize the use of automated tooling from the cloud-dataproc repository instead of manual gcloud commands. It introduces comprehensive sections on network evaluation, enhanced proxy support (including custom CA certificates), and building custom images for Secure Boot. Additionally, the metadata parameter documentation is expanded, and the tested image version matrix is updated. Reviewer feedback focuses on improving clarity by reverting node terminology, ensuring JSON examples are valid and properly placed, and maintaining consistency in metadata documentation and section headers.
| This initialization action installs GPU driver for NVIDIA GPUs on -m node(s) and | ||
| -w nodes in a Dataproc cluster. |
There was a problem hiding this comment.
The use of -m node(s) and -w nodes is less clear for a general audience than the original master and worker nodes. While these flags are common in gcloud commands, a README file benefits from being as explicit as possible. For better readability, I recommend using the full terms master and worker.
| This initialization action installs GPU driver for NVIDIA GPUs on -m node(s) and | |
| -w nodes in a Dataproc cluster. | |
| This initialization action installs GPU driver for NVIDIA GPUs on master and | |
| worker nodes in a Dataproc cluster. |
| * Copy the sample configuration: `cp env.json.sample env.json` | ||
| * Edit `env.json` to match your desired cluster setup. | ||
|
|
||
| **Note on JSON Examples:** Any lines in the JSON example below starting with `//` are comments for explanation and should be removed before using the JSON. |
There was a problem hiding this comment.
This note regarding JSON examples is helpful, but it is located very far from the JSON example it refers to (around line 308). This could be confusing for readers. It would improve clarity to move this note to directly precede the relevant JSON code block.
|
|
||
| * **Configuration:** Use the `http-proxy`, `https-proxy`, or `proxy-uri` metadata to specify your proxy server (host:port). | ||
| * **Custom CA Certificates:** If your proxy uses a custom CA (e.g., self-signed), provide the CA certificate in PEM format via the `http-proxy-pem-uri` metadata (as a `gs://` path). | ||
| * **Integrity Check:** Optionally, provide the SHA256 hash of the PEM file via `http-proxy-pem-sha256` to ensure the downloaded file is correct. |
There was a problem hiding this comment.
This section introduces the http-proxy-pem-sha256 metadata parameter. For consistency and discoverability, this new parameter should also be documented in the main 'Metadata Parameters' list (around line 202).
| // Add these for a private, proxied environment | ||
| "PRIVATE_RANGE": "10.43.79.0/24", | ||
| "SWP_RANGE": "10.44.79.0/24", | ||
| "SWP_IP": "10.43.79.245", | ||
| "SWP_PORT": "3128", | ||
| "SWP_HOSTNAME": "swp.your-project.example.com" | ||
| // ... other variables as needed |
There was a problem hiding this comment.
The JSON example provided contains comments, which makes it invalid and prevents users from copying and pasting it directly. While there is a note about this earlier in the document, it's best practice to provide valid code snippets. Please consider moving the explanatory comments into the surrounding text to make the JSON block valid.
| // Add these for a private, proxied environment | |
| "PRIVATE_RANGE": "10.43.79.0/24", | |
| "SWP_RANGE": "10.44.79.0/24", | |
| "SWP_IP": "10.43.79.245", | |
| "SWP_PORT": "3128", | |
| "SWP_HOSTNAME": "swp.your-project.example.com" | |
| // ... other variables as needed | |
| "PRIVATE_RANGE": "10.43.79.0/24", | |
| "SWP_RANGE": "10.44.79.0/24", | |
| "SWP_IP": "10.43.79.245", | |
| "SWP_PORT": "3128", | |
| "SWP_HOSTNAME": "swp.your-project.example.com" |
| bash bin/create-dpgce-private | ||
| ``` | ||
|
|
||
| **Verification:** |
There was a problem hiding this comment.
There appears to be a duplicate 'Verification' section. Here it is formatted as bold text, while on line 341 it's a level-3 header (### Verification). This is structurally inconsistent and could be confusing. To improve the document's structure, consider changing this to a sub-header, such as #### Verification.
| **Verification:** | |
| #### Verification: |
1 participant
testing a simple commit to see if tests pass