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

Add op-determinism info to version 2.7 release notes #52971

Merged
merged 1 commit into from Nov 10, 2021

Conversation

duncanriach
Copy link
Contributor

This is a PR for the r2.7 branch.

This PR represents release notes that should have been included with version 2.7. I failed to get this completed before the release. I'm hoping that this information can be added to the release notes on GitHub and included in the master branch version of RELEASE.md.

Please will someone with experience in this area, such as @goldiegadde, direct a procedure for making sure this information lands in the correct place(s).

Before this information is propagated, please will @reedwm and @pkanwar23 review for correctness and completeness. Note that there is a "TODO: confirm exception added" that needs to be removed with respect to tf.math.bincount.

@google-cla google-cla bot added the cla: yes label Nov 6, 2021
@duncanriach
Copy link
Contributor Author

@goldiegadde, the change to the outside-Google contributions list was just to add my name, which was/is missing. I wonder if others are missing.

@mihaimaruseac
Copy link
Collaborator

mihaimaruseac commented Nov 7, 2021

So, the process for the release notes is as follows:

  • Before branch cut, when you make PR, you can also edit RELEASE.md to include the adequate notes
  • After branch cut:
    • If making a new PR with the goal to be cherrypicked: don't add to RELEASE.md, when doing the cherrypick for the PR add to the RELEASE.md on the branch
    • If some release notes are missing on the branch, make a PR against the branch (just like this one) to update
  • For each RC, the release notes in the GitHub release are the contents of RELEASE.md at that commit (same for final release)
  • After final release: RELEASE.md from the branch gets updated on master branch

Now, if we take this PR, it will be included in a future patch release. However, for patch releases we have separate release notes, so it won't show up there either.

@gbaned gbaned self-assigned this Nov 8, 2021
@gbaned gbaned added the size:S CL Change Size: Small label Nov 8, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Nov 8, 2021
@gbaned gbaned assigned mihaimaruseac and unassigned gbaned Nov 8, 2021
@duncanriach
Copy link
Contributor Author

duncanriach commented Nov 10, 2021

Thank you, @mihaimaruseac. I'm very grateful for you providing a comprehensive roadmap for updating release notes. In the past, it's been challenging for me to figure out what to do. I'm wondering if these procedures are now recorded somewhere central and permanent.

From what you wrote above, my understanding is that even if this PR is approved and merged, its contents will be lost in the r2.7 branch RELEASE.md, and will never be visible in the GitHub release notes, even for the patch releases. My understanding is also that changes will not get into the master branch because we're past the final release (2.7.0). This is totally my mistake; in the past I have been more punctual. If these things are true, then it makes this PR essentially pointless. I guess this also applies to @nouiz's 52990.

So that we don't just bin this useful info, what are your thoughts on us adding these changes (instead) to the master branch's RELEASE.md for version 2.8 (perhaps with a note indicating that these changes were actually in version 2.7 onwards)?

Also, even though I judge myself for being so vain, and I know that all the wonderful folks at Google don't get their names on the release notes, I am a little bit miffed that I contributed so much to version 2.7, but, strangely, my name was omitted from the contributors list. Do you know what happened? Have other people's names been omitted? I'm super grateful for all your hard work, and I hope I'm not behaving inappropriately entitled, but I would like to understand what happened with this.

PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Nov 10, 2021
@mihaimaruseac
Copy link
Collaborator

You raise good questions.

I'm going to merge these two PRs and then manually update the GitHub relnotes to sync and also sync on master.

Probably it's also better to include these in a 2.8 section with a mention that they were added in 2.7 to make sure people get a chance to find about them if they no longer read 2.7 section.

Regarding why the name was omitted, I think that's a bug in our infra. I'll file an internal bug to investigate.

@mihaimaruseac mihaimaruseac merged commit 4d72543 into tensorflow:r2.7 Nov 10, 2021
@reedwm
Copy link
Member

reedwm commented Nov 10, 2021

Before this information is propagated, please will @reedwm and @pkanwar23 review for correctness and completeness

Thanks @duncanriach for the PR, and thanks @mihaimaruseac for merging this and explaining the situation. Sorry for not looking at this earlier. The release notes changes look good. Two minor points:

  • The release notes say tf.gather backprop is made deterministic. f29ef28 fixes a crash with tf.gather backprop but it was already deterministic (and that commit barely missed 2.7, but will be in 2.8). There might be some other fix to tf.gather backprop that I'm unaware of or forgot about though.
  • There is a TODO to confirm an exception to bincount was added. I can confirm it was, in f0e6c2f.

Neither of these points are major so I don't think we have to worry about them if we don't want to deal with them.

@duncanriach
Copy link
Contributor Author

Thank you for doing all of that, @mihaimaruseac.

@reedwm, the tf.gather backprop note is related to the addition of deterministic tf.math.unsorted_segment_sum. Recall that tf.gather backprop produces tf.IndexedSlices, which are reduced, using tf.math.unsorted_segment_sum, back into the dense params tensor from where they were gathered. So, even though the backprop of tf.gather itself is technically deterministic, I believe that essentially all users would experience the backprop of tf.gather as functioning nondeterministically, under the conditions that lead to the subsequent, associated nondeterministic reduction.

@duncanriach duncanriach deleted the release-notes-v2.7 branch January 10, 2022 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes size:S CL Change Size: Small
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

4 participants