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

[determinism] Add v2.8 release notes #53465

Conversation

duncanriach
Copy link
Contributor

Add op-determinism changes to the version 2.8 release notes in the master branch (prior to the r2.8 branch cut).

@reedwm, please will you review. This is not much different than the notes for version 2.7. I'm wondering if there any additional enhancements in version 2.8 that I'm not yet aware of.

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Dec 16, 2021
@reedwm
Copy link
Member

reedwm commented Dec 16, 2021

Yesterday I added a brief section in the release notes, in d5b3681, but it doesn't list the 2.7 changes. I don't think it is worth listing the 2.7 changes for TF 2.8 despite having not listing them in the TF 2.7 release, since there are no known ops that are nondeterministic and do not raise an error. So users don't need to be aware of which ops are nondeterministic by reading the release notes, because they all should be deterministic. Granted, there are almost certainly bugs that still will cause nondeterminism, but hopefully such bugs will be rare.

I have a small XLA change that makes XLA aware of the TF global determinism variable, which will allow determinism to work correctly when tf.function(jit_compile=True) is used. I'm hoping it will be submitted within an hour of now, by the branch cut, and I'll let you know when it is submitted. I also added exceptions to some FakeQuant ops in ae1581f. Unfortunately, my changes to make tf.data faster with determinism haven't landed yet.

Anyway, I'm happy to use your wording of the release notes, or keep my wording, but I don't think we should list 2.7 changes. It does make sense to mention the 2.8 changes though, so feel free to add those in, whether we go with my wording or yours.

@reedwm
Copy link
Member

reedwm commented Dec 16, 2021

I'm hoping it will be submitted within an hour of now, by the branch cut, and I'll let you know when it is submitted.

Actually looks like the change already landed 5 minutes ago, in 4dc6d4d.

@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Dec 17, 2021
@gbaned gbaned requested a review from reedwm December 17, 2021 12:57
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label Dec 17, 2021
@duncanriach
Copy link
Contributor Author

@reedwm, I'm sorry I missed your addition to the release notes.

I have merged your change with mine in this PR. I have also added notes about the two changes you listed above. Thank you for sharing. I put it in the "Major Features and Improvements" section because it's a new, official API feature.

When possible, I would like to document the incremental changes for this functionality in the release notes. For example, some users are likely going to care, going forward, when an exception is replaced with an implementation. Also, if it makes sense to include the changes that are 2.8-specific then why not include the changes that were not reported for the 2.7 release? I tend to err on the side of being detail-oriented about tracking and reporting these changes because I'm trying to make this functionality as explicit and thoroughly documented as possible (for those who care), to minimize potential confusion.

@reedwm
Copy link
Member

reedwm commented Dec 22, 2021

Agreed we should document incremental changes. I was a bit reluctant to include 2.7 changes in the 2.8 release notes, but you're right that this makes sense since we didn't report them in 2.7.

I pushed a commit to this PR that keeps the enable_op_determinism function in the "Major Features" section but moves the individual op changes to the "Bug Fixes and Other Changes" section. Since determinism was previously inaccessible through a public API, the entirety of the determinism work can be considered a major new feature, so it's not worth listing the individual determinism changes in that section IMO.

reedwm
reedwm previously approved these changes Dec 22, 2021
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Dec 22, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Dec 22, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 22, 2021
@mihaimaruseac
Copy link
Collaborator

We cut the branch for TF 2.8 now. Can you rebase this on r2.8 branch?

Apologies for the conflict, but if we keep this against master then the changes will be missed after the release when we merge the release notes from the branch back into master.

@reedwm
Copy link
Member

reedwm commented Dec 22, 2021

I think the PR author may be offline until January, and I'm not sure if it's possible for me to rebase a PR that I didn't author (although I can create commits). Can I create a cherrypick into 2.8 instead?

If necessary, I can create a new PR as well.

@mihaimaruseac mihaimaruseac changed the base branch from master to r2.8 December 22, 2021 20:08
@mihaimaruseac mihaimaruseac dismissed reedwm’s stale review December 22, 2021 20:08

The base branch was changed.

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Dec 22, 2021
@mihaimaruseac mihaimaruseac changed the base branch from r2.8 to master December 22, 2021 20:09
@google-ml-butler google-ml-butler bot added the kokoro:force-run Tests on submitted change label Dec 22, 2021
PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Dec 22, 2021
@mihaimaruseac
Copy link
Collaborator

Yes, let's land in master and then do a cherrypick. Sorry it needs extra work, I should have checked these a few days ago

@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 22, 2021
@copybara-service copybara-service bot merged commit 1a0ead1 into tensorflow:master Dec 22, 2021
@google-ml-butler google-ml-butler bot removed the awaiting review Pull request awaiting review label Dec 22, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Dec 22, 2021
mihaimaruseac pushed a commit that referenced this pull request Dec 23, 2021
…n-master-branch

PiperOrigin-RevId: 417866493
Change-Id: I23fc1c45ae2cfa0b8758326440320c29ced79c60
@duncanriach duncanriach deleted the add-d9m-to-r2.8-relnotes-in-master-branch branch January 10, 2022 23:54
@duncanriach
Copy link
Contributor Author

Thank you, @reedwm. Nice improvements. Also, thanks for making that cherry-pick happen. Note that some of the indentation got lost somewhere, which PR 53720 addresses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:S CL Change Size: Small
Projects
PR Queue
  
Approved by Reviewer
Development

Successfully merging this pull request may close these issues.

None yet

5 participants