Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup in prep for structured params #89

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

klueska
Copy link
Collaborator

@klueska klueska commented Mar 15, 2024

No description provided.

@klueska klueska requested a review from elezar March 15, 2024 12:04
@@ -0,0 +1,19 @@
/*
* Copyright (c) 2023, NVIDIA CORPORATION. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question. What is a consistent year for these headers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, not sure. This is obviously a copy and paste job ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR. More a general question for follow-up.

@@ -14,7 +14,7 @@
* limitations under the License.
*/

package v1alpha1
package sharing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these utils are no longer versioned? Is that because we don't expect them to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you're right. They are in fact public, which is why they should probably be versioned....

Copy link
Collaborator Author

@klueska klueska Mar 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it back -- the apis that use this are still versioned, so as long as we only change this in conjunction with a bump on those API versions, we are OK.

@@ -167,6 +175,8 @@ func (l deviceLib) getGpuInfo(index int, device nvdev.Device) (*GpuInfo, error)
brand: brand,
architecture: architecture,
cudaComputeCapability: cudaComputeCapability,
driverVersion: driverVersion,
cudaDriverVersion: fmt.Sprintf("%v.%v", cudaDriverVersion/1000, (cudaDriverVersion%1000)/10),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope for this PR. But do we want to pull this into gonvlib (GetCUDADriverVersionString or something?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let#s do it as a follow-up

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some general questions / comments. No blockers.

@klueska klueska merged commit 9fb92ad into NVIDIA:main Mar 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants