Skip to content
This repository has been archived by the owner on Jul 1, 2023. It is now read-only.

Convert required proto field to optional. #956

Merged
merged 1 commit into from
May 13, 2020
Merged

Convert required proto field to optional. #956

merged 1 commit into from
May 13, 2020

Conversation

pschuh
Copy link
Contributor

@pschuh pschuh commented May 13, 2020

new protobuf fields should not be required.

see: protocolbuffers/protobuf#2497

@pschuh pschuh requested a review from asuhan May 13, 2020 19:53
asuhan
asuhan approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@asuhan asuhan left a comment

Choose a reason for hiding this comment

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

I don't mind it, but not sure I understand the argument - uid was in the GetNcclUniqueUidResponse from the very beginning, I thought optional was meant for backward compatibility.

@pschuh pschuh merged commit 4d2b68e into tensorflow:master May 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@asuhan asuhan asuhan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pschuh @asuhan