Skip to content

Conversation

elezar
Copy link
Member

@elezar elezar commented Jun 19, 2024

These changes move from using nvml.Return values to using native Golang error values, making code more idiomatic.

Note that this requires that error.Is is used instead of checking for equality in the returned value.

For example:

current, pending, err := device.GetMigMode()
if errors.Is(err, nvml.ERROR_NOT_SUPPORTED) {
	fmt.Printf("MIG is not supported for device %v\n", i)

} else if err != nil {
	log.Fatalf("Error getting MIG mode for device at index %d: %v", i, err)
}
fmt.Printf("MIG mode for device %v: current=%v pending=%v\n", i, current, pending)

would become:

current, pending, err := device.GetMigMode()
if errors.Is(err, nvml.ERROR_NOT_SUPPORTED) {
	fmt.Printf("MIG is not supported for device %v\n", i)

} else if err != nil {
	log.Fatalf("Error getting MIG mode for device at index %d: %v", i, err)
}
fmt.Printf("MIG mode for device %v: current=%v pending=%v\n", i, current, pending)

@elezar elezar requested a review from klueska July 9, 2024 12:02
@elezar elezar force-pushed the return-go-error branch from bbfddf5 to 6dde3b8 Compare July 9, 2024 12:14
@elezar elezar force-pushed the return-go-error branch from 6dde3b8 to 98e003d Compare July 9, 2024 12:21
@klueska
Copy link
Contributor

klueska commented Jul 10, 2024

Your two example's in the description are the same .. I think you wanted one of them to check euqility instead of using errors.Is().

That said -- what will happen if someone does do err == nvml.ERROR_NOT_SUPPORTED? Will it be false even when errors.Is(err, nvml.ERROR_NOT_SUPPORTED) would be true?

Comment on lines +56 to +63
current, pending, err := device.GetMigMode()
if errors.Is(err, nvml.ERROR_NOT_SUPPORTED) {
fmt.Printf("MIG is not supported for device %v\n", i)

} else if err != nil {
log.Fatalf("Error getting MIG mode for device at index %d: %v", i, err)
}
fmt.Printf("MIG mode for device %v: current=%v pending=%v\n", i, current, pending)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to:

		current, pending, err := device.GetMigMode()
		if err == nil {
			fmt.Printf("MIG mode for device %v: current=%v pending=%v\n", i, current, pending)
			continue
		}
		if errors.Is(err, nvml.ERROR_NOT_SUPPORTED) {
			fmt.Printf("MIG is not supported for device %v\n", i)
			continue
		}
		log.Fatalf("Error getting MIG mode for device at index %d: %v", i, err)

Copy link
Contributor

Choose a reason for hiding this comment

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

None of the changes in this file look intentional...

Comment on lines 22 to 24
func (l *library) ErrorString(r Return) string {
return r.Error()
return r.String()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How does an end-user ever get ahold of a Return value to pass it to this function?

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