Skip injection of nvidia-persistenced socket by default #694
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elezar
force-pushed
the
add-opt-in-to-sockets
branch
from
e1cfb49
to
f44e31f
Compare
cdesiniotis
approved these changes
Sep 16, 2024
shivamerla
reviewed
Sep 17, 2024
|
|
||
| // Opt out of the fabricmanager and persistenced sockets if these are not | ||
| // explicitly enabled. | ||
| if !hook.Features.AllowFabricmanager.IsEnabled() { |
There was a problem hiding this comment.
So, containers depending on this socket (e.g. NCCL library) will error out after this update by default? This will impact multi-node training Jobs etc.
There was a problem hiding this comment.
Changing the scope of this PR to only nvidia-persistenced.
klueska
previously requested changes
Sep 18, 2024
elezar
force-pushed
the
add-opt-in-to-sockets
branch
2 times, most recently
from
d517584
to
fa0ec9d
Compare
elezar
changed the title
This changes skips the injection of the nvidia-persistenced socket by default. An include-persistenced-socket feature flag is added to allow the injection of this socket to be explicitly requested. Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
force-pushed
the
add-opt-in-to-sockets
branch
from
fa0ec9d
to
d2deb77
Compare
cdesiniotis
reviewed
Sep 18, 2024
cdesiniotis
reviewed
Sep 18, 2024
cdesiniotis
approved these changes
Sep 18, 2024
This change ensures that the internal CDI representation includes the persistenced socket if the include-persistenced-socket feature flag is enabled. Signed-off-by: Evan Lezar <elezar@nvidia.com>
This change adds an include-persistenced-socket flag to the nvidia-ctk cdi generate command that ensures that a generated specification includes the nvidia-persistenced socket if present on the host. Note that for mangement mode, these sockets are always included if detected. Signed-off-by: Evan Lezar <elezar@nvidia.com>
This change enables opt-in (off-by-default) features to be opted into. These features can be toggled by name by specifying the (repeated) --opt-in-feature command line argument or as a comma-separated list in the NVIDIA_CONTAINER_TOOLKIT_OPT_IN_FEATURES environment variable. Signed-off-by: Evan Lezar <elezar@nvidia.com>
cdesiniotis
requested changes
Sep 18, 2024
elezar
force-pushed
the
add-opt-in-to-sockets
branch
from
baaaab3
to
9c2476c
Compare
cdesiniotis
approved these changes
Sep 18, 2024
elezar
dismissed
klueska’s stale review
addressed and re-reviewed by @cdesiniotis
elezar
added a commit
to elezar/nvidia-container-toolkit
that referenced
this pull request
Sep 20, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes skips the injection of the nvidia-persistenced socket by default.
An include-persistenced-socket feature flag is added to allow the
injection of this socket to be explicitly requested.
See NVIDIA/libnvidia-container#283