Conversation
28c8045 to
de44464
Compare
cdesiniotis
left a comment
There was a problem hiding this comment.
LGTM. @varunrsekar can we add unit tests to confirm the new profiles get parsed correctly?
de44464 to
a6d1ae0
Compare
Signed-off-by: Varun Ramachandra Sekar <vsekar@nvidia.com>
a6d1ae0 to
30bc37a
Compare
@cdesiniotis Thanks for the review. Earlier, I had missed adding the |
| sort.Strings(attrs1) | ||
| sort.Strings(attrs2) |
There was a problem hiding this comment.
Not that this is different from the previous implementation, but this sorts the attributes in place meaning that if a device has multiple attributes the profile name may change.
There was a problem hiding this comment.
We should revisit what it means to have multiple attributes. Currently no MIG profiles support it. So hard to imagine what the expectation is.
| // Split by +/- to separate out attributes. | ||
| split := strings.SplitN(profile, "+", 2) | ||
| negsplit := strings.SplitN(profile, "-", 2) | ||
| // Make sure we don't get both positive and negative attributes. |
There was a problem hiding this comment.
@elezar I've mostly tried to fit into the existing implementation.
But looking at the existing profiles, we don't have multi-attribute profiles yet.
+me profiles: Include at least one media engine (NVDEC, NVENC, NVJPG, or OFA).
+gfx: Adds support for graphics APIs (new in GB20X).
+me.all: Allocates all available media engines to this instance (does not include graphics support).
-me: Excludes all media engines for pure compute workloads.
I would've preferred the attributes to me named +me, +me.all, -me etc instead so we don't have this differentiation.
There was a problem hiding this comment.
But looking at the existing profiles, we don't have multi-attribute profiles yet.
When we do have multiple attributes, would we expect the + (or -) to be included in the name? Does that mean that we would have +me,+gfx or +me,gfx, for example?
There was a problem hiding this comment.
We shouldn't have to decide this. We should just adopt whatever the output of nvidia-smi shows for these profile names.
Signed-off-by: Varun Ramachandra Sekar <vsekar@nvidia.com>
|
@varunrsekar Thanks for this PR. I have a minor comment. In the |
| return nil, fmt.Errorf("attribute begins with a number") | ||
| } | ||
| if a[0] == '.' || a[len(a)-1] == '.' { | ||
| return nil, fmt.Errorf("attribute begins/ends with a dot") |
There was a problem hiding this comment.
| return nil, fmt.Errorf("attribute begins/ends with a dot") | |
| return nil, fmt.Errorf("attribute %s begins/ends with a period/dot", a) |
There was a problem hiding this comment.
@tariq1890 The new errors are following existing pattern. The fmt.Errorf/fmt.Printf fixes you requested can be addressed in a follow-up and keep the focus on the new profiles in this PR.
There was a problem hiding this comment.
Sure, I won't block the PR on this comment. But generally, I prefer making improvments on the lines we touch regardless of the conventions followed in the existing code. This practice helps make incremental improvements possible
At the bare minimum, this change is needed to run
device.NewMigProfilefor the new MIG profiles (9-16)