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.

Sign up for GitHub

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

Jump to bottom

Fix #3288: Add model layer for checkpointing #3271

Merged
merged 14 commits into from
Jun 17, 2021
Merged

Fix #3288: Add model layer for checkpointing #3271

merged 14 commits into from
Jun 17, 2021

Conversation

MaskedCarrot
Copy link
Contributor

@MaskedCarrot MaskedCarrot commented Jun 7, 2021 •
edited

Explanation

Fixes #3288

Updated the protobufs to implement checkpointing.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

BenHenning
BenHenning previously requested changes Jun 9, 2021
View reviewed changes
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Adding one comment since the current implementation will break existing progress saved on devices.

model/src/main/proto/topic.proto Outdated Show resolved Hide resolved
@oppiabot
Copy link

oppiabot bot commented Jun 9, 2021

Hi @MaskedCarrot, it looks like some changes were requested on this pull request by @BenHenning. PTAL. Thanks!

@MaskedCarrot MaskedCarrot removed their assignment Jun 10, 2021
@aggarwalpulkit596 aggarwalpulkit596 marked this pull request as ready for review June 10, 2021 19:12
@oppiabot
Copy link

oppiabot bot commented Jun 10, 2021

Assigning @vinitamurthi for code owner reviews. Thanks!

@oppiabot
Copy link

oppiabot bot commented Jun 11, 2021

Hi @MaskedCarrot, it looks like some changes were requested on this pull request by @aggarwalpulkit596. PTAL. Thanks!

@aggarwalpulkit596
Copy link
Contributor

@MaskedCarrot there seems to be one unit test failing for Bazel could you please check that.

@aggarwalpulkit596
Copy link
Contributor

@vinitamurthi @rt4914 could you follow up with your comments here.

rt4914
rt4914 approved these changes Jun 15, 2021
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@rt4914 rt4914 removed their assignment Jun 15, 2021
@aggarwalpulkit596
Copy link
Contributor

@MaskedCarrot could you please update on this test case which is failing?

aggarwalpulkit596
aggarwalpulkit596 approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@aggarwalpulkit596 aggarwalpulkit596 left a comment

Choose a reason for hiding this comment

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

LGTM

@MaskedCarrot
Copy link
Contributor Author

@aggarwalpulkit596 I am not sure why this test is failing.

@aggarwalpulkit596
Copy link
Contributor

@anandwana001 As per the conversation the failing test are flaky test so i'm merging the PR.

@aggarwalpulkit596 aggarwalpulkit596 enabled auto-merge (squash) June 16, 2021 14:48
@aggarwalpulkit596 aggarwalpulkit596 dismissed BenHenning’s stale review June 16, 2021 14:49

The changes have been addressed and since ben availability is limited this week i'm dismissing the review for now.

@aggarwalpulkit596
Copy link
Contributor

@vinitamurthi Could you please approve the PR if there aren't any changes required? Thanks

@aggarwalpulkit596 aggarwalpulkit596 enabled auto-merge (squash) June 16, 2021 14:51
vinitamurthi
vinitamurthi approved these changes Jun 17, 2021
View reviewed changes
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@aggarwalpulkit596 aggarwalpulkit596 merged commit 1cc3fc9 into oppia:develop Jun 17, 2021
@MaskedCarrot MaskedCarrot deleted the change-proto-for-checkpointing branch July 8, 2021 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@vinitamurthi vinitamurthi vinitamurthi approved these changes

@fsharpasharp fsharpasharp fsharpasharp approved these changes

@aggarwalpulkit596 aggarwalpulkit596 aggarwalpulkit596 approved these changes

@BenHenning BenHenning Awaiting requested review from BenHenning

Assignees

@vinitamurthi vinitamurthi

@BenHenning BenHenning

@MaskedCarrot MaskedCarrot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create model layer for checkpointing
6 participants
@MaskedCarrot @aggarwalpulkit596 @rt4914 @vinitamurthi @BenHenning @fsharpasharp