Improve README.md #107
Improve README.md #107
Conversation
README.md
Outdated
| @@ -32,15 +32,20 @@ First since we'll launch kind with GPU support, ensure that the following prereq | |||
| Container Runtime to use volume mounts to select devices to inject into a | |||
| container. | |||
|
|
|||
| 1. Show the current GPU information of the machine | |||
There was a problem hiding this comment.
I think this step is covered at https://docs.nvidia.com/datacenter/cloud-native/container-toolkit/install-guide.html)
There was a problem hiding this comment.
Yes, but would it be helpful to have it in the root README? For example, I was following the instructions in this README in my first attempt.
README.md
Outdated
| We start by first cloning this repository and `cd`ing into it. | ||
| All of the scripts and example Pod specs used in this demo are in the `demo` | ||
| subdirectory, so take a moment to browse through the various files and see | ||
| what's available: | ||
|
|
||
| ``` | ||
| ```bash |
There was a problem hiding this comment.
I see bash/console should we agree on a single one? is there a benefit of having one vs the other on specific steps? (is one better for copy/pasting than the other for example?
There was a problem hiding this comment.
Changed all of them to console.
README.md
Outdated
| ```console | ||
| $ kubectl get pods -n nvidia-dra-driver | ||
| ``` | ||
| ``` |
There was a problem hiding this comment.
What is the purpose of separating this if you keep the $?
There was a problem hiding this comment.
A user can copy/paste and run the command directly.
There was a problem hiding this comment.
Not with the $still there.
There was a problem hiding this comment.
A user can copy/paste and run an example command directly.
There was a problem hiding this comment.
I'm trying to say that you didn't remove the $ in your diff.
| $ kubectl logs -n gpu-test1 -l app=pod | ||
| kubectl logs -n gpu-test1 -l app=pod | ||
| ``` | ||
| ``` |
There was a problem hiding this comment.
I looked at the generated README with this change, and I prefer the look of the original. Is the purpose here to make it easier to copy-and-paste the command where the output is attached?
There was a problem hiding this comment.
Yes, otherwise, a user has to manually delete the character $. Also some examples contain the output of a command in the console quote, which makes the command invalid.
README.md
Outdated
| @@ -58,13 +63,15 @@ From here we will build the image for the example resource driver: | |||
| This also makes the built images available to the `kind` cluster. | |||
|
|
|||
| We now install the NVIDIA GPU DRA driver: | |||
| ``` | |||
| ```bash | |||
There was a problem hiding this comment.
Why sometimes bash and sometimes console?
There was a problem hiding this comment.
It's inherited from the original script. Let's use console, which I believe is what's used in k8s.
yuanchen8911
force-pushed
the
patch-1
branch
2 times, most recently
from
386af85 to
1a974c1
Compare
README.md
Outdated
| 1. Set the `accept-nvidia-visible-devices-as-volume-mounts` option to `true` in | ||
| the `/etc/nvidia-container-runtime/config.toml` file to configure the NVIDIA | ||
| Container Runtime to use volume mounts to select devices to inject into a | ||
| container. | ||
|
|
There was a problem hiding this comment.
While you're changing things do you want to add this command below this step as well:
sudo nvidia-ctk config --set accept-nvidia-visible-devices-as-volume-mounts=true --in-place
README.md
Outdated
| Where the running the first three examples should produce output similar to the following: | ||
| Depending on which GPUs are available, running the first three examples will produce output similar to the following: | ||
| ```console | ||
| $ kubectl apply --filename=demo/specs/quickstart/gpu-test{1,2,3}.yaml | ||
| ... | ||
|
|
||
| kubectl apply --filename=demo/specs/quickstart/gpu-test{1,2,3}.yaml | ||
| ``` | ||
|
|
||
| ```console | ||
| $ kubectl get pod -A | ||
| kubectl get pod -A -l app=pod |
There was a problem hiding this comment.
This doesn't read well after this change. Can you update the text to first describe running the apply call and then running kubectl get to see them starting up.
README.md
Outdated
| $ ./demo/clusters/kind/delete-cluster.sh | ||
| ./demo/clusters/kind/delete-cluster.sh |
This PR improves the instructions in the README file by 1. Fixing typos in the example `kubectl` commands by removing the extra `$` at the beginning and the command output. 2. Adding the command `nvidia-smi -L` to show the GPU information on the node. Signed-off-by: Yuan Chen <[email protected]> Update README.md Signed-off-by: Yuan Chen <[email protected]> Update README.md Signed-off-by: Yuan Chen <[email protected]> Update README.md Signed-off-by: Yuan Chen <[email protected]> Update README.md Signed-off-by: Yuan Chen <[email protected]> Update README.md Co-authored-by: Kevin Klues <[email protected]> Signed-off-by: Yuan Chen <[email protected]> Update README.md Co-authored-by: Kevin Klues <[email protected]> Signed-off-by: Yuan Chen <[email protected]> Apply Kevin's changes Remove $ Update Add additional command
|
Thanks for the comments. I think it's looking much better now. |
|
There's still some things worth changing, but this is definitely better than before. |
klueska
dismissed
ArangoGutierrez’s stale review
The changes requested have been addressed.
This PR improves the instructions in the README file by
kubectlcommands by removing the extra$at the beginning and the command output. So a user can directly copy and paste the example commands to run.nvidia-smi -Lto show the GPU information on the node.