Fix that only allows creation of ModelRollouts with RolloutPeriodStartTime after that of previous ModelRollout #1076

jojijac0b · 2023-06-22T19:45:32Z

Update query in readModelRolloutData to return the ModelRollout for a given ModelLine with the latest RolloutPeriodStartTime in CreateModelRollout.kt. Update CreateModelRollout.kt to throw ModelRolloutInvalidArgsException if the RolloutPeriodStartTime of the ModelRollout to be created precedes that of the last ModelRollout. Create tests in ModelRolloutServiceTest.kt. Update 'createModelRollout succeeds with multiple model releases' test to adhere to new RolloutPeriodStartTime limitations.

… given ModelLine with the latest RolloutPeriodStartTime in CreateModelRollout.kt. Update CreateModelRollout.kt to throw ModelRolloutInvalidArgsException if the RolloutPeriodStartTime of the ModelRollout to be created precedes that of the last ModelRollout. Create tests in ModelRolloutServiceTest.kt

wfa-reviewable · 2023-06-22T19:45:40Z

This change is

SanjayVas

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jojijac0b, @Marco-Premier, and @tristanvuong2021)

a discussion (no related file):
Would you mind also documenting this restriction in the API definition?

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 183 at r1 (raw file):

  }

  private suspend fun TransactionScope.readModelRolloutData(

Perhaps this should be renamed to indicate that it's reading data for some previous ModelRollout? Also document what is meant by "previous" since that can be ambiguous. It looks like in this case it means the one for the given ModelLine with the least recent RolloutPeriodStartTime.

Suggestion:

readPreviousModelRolloutData

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 187 at r1 (raw file):

    externalModelSuiteId: ExternalId,
    externalModelLineId: ExternalId,
  ): Struct? {

nit: Prefer converting to some more specific type immediately rather than consuming a free-from structure like Struct. e.g. create a private data class for this purpose.

Code quote:

Struct

…sely reflect what is done in the function. Create data type to be returned by readLatestModelRolloutData. Add comments to explain why the error is thrown. Add comments to API definition to surface this limitation.

jojijac0b

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @SanjayVas, and @tristanvuong2021)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Would you mind also documenting this restriction in the API definition?

Commented limitation in API definition in model_rollouts_service.proto

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 183 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Perhaps this should be renamed to indicate that it's reading data for some previous ModelRollout? Also document what is meant by "previous" since that can be ambiguous. It looks like in this case it means the one for the given ModelLine with the least recent RolloutPeriodStartTime.

Renamed to readLatestModelRolloutData to reflect the query returning the ModelRollout with the latest RolloutPeriodStartTime.

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 187 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: Prefer converting to some more specific type immediately rather than consuming a free-from structure like Struct. e.g. create a private data class for this purpose.

Created data class LatestModelRolloutResult

SanjayVas

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jojijac0b, @Marco-Premier, and @tristanvuong2021)

a discussion (no related file):

Previously, jojijac0b wrote…

Commented limitation in API definition in model_rollouts_service.proto

I had meant a follow-up PR in the public API definition in the cross-media-measurement-api repo.

src/main/proto/wfa/measurement/internal/kingdom/model_rollouts_service.proto line 27 at r2 (raw file):

// Internal service for persistence of ModelRollout entities.
service ModelRollouts {
  // rollout_period_start_time of given `ModelRollout` must be later than that

Documentation should start with a summary fragment. e.g. "Creates a new ModelRollout."

Marco-Premier

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jojijac0b and @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ModelRolloutsServiceTest.kt line 1051 at r2 (raw file):

  @Test
  fun `createModelRollout succeeds with multiple model releases`() = runBlocking {

nit multiple model rollouts

Code quote:

model releases

…with multiple model rollouts. Remove comment in API definition(will create another PR in public API).

jojijac0b

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @Marco-Premier, @SanjayVas, and @tristanvuong2021)

a discussion (no related file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I had meant a follow-up PR in the public API definition in the cross-media-measurement-api repo.

Deleted comment in model_rollouts_service.proto. Will create PR in public API.

src/main/proto/wfa/measurement/internal/kingdom/model_rollouts_service.proto line 27 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Documentation should start with a summary fragment. e.g. "Creates a new ModelRollout."

Done.

tristanvuong2021

Reviewed 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jojijac0b and @SanjayVas)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

    val latestModelRolloutData =
      if (modelRollout.rolloutPeriodStartTime != modelRollout.rolloutPeriodEndTime) {

How does this case work with the restriction? Instant rollouts can be scheduled whenever? I am thinking about when an instant rollout is scheduled before the latest gradual rollout so the latest gradual rollout still has its previous rollout as the rollout before the instant rollout. If instant rollouts can be scheduled whenever, should the rollout right after the instant rollout have its previous rollout updated to be the instant rollout?

SanjayVas

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):
As an aside, I want to call out that TimeInterval had incorrect documentation added in world-federation-of-advertisers/cross-media-measurement-api#130. The meaning of TimeInterval did not change in that PR, meaning this line is incorrect:

If equal it represents an exact point in time.

TimeInterval was just our own version of google.type.Interval, which had not yet been published when we initially created the API definition. See world-federation-of-advertisers/cross-media-measurement-api#151

SanjayVas

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):
Relevant line from the google.type.Interval documentation:

When the start equals the end, the interval is empty (matches no time).

Marco-Premier · 2023-06-26T17:44:53Z

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Relevant line from the google.type.Interval documentation:

When the start equals the end, the interval is empty (matches no time).

That is true @tristanvuong2021 .
Instant rollouts cannot be schedule whenever. They must follow the same logic for rollouts with rollout_period.
@jojijac0b this should become:

val latestModelRolloutData =
  readLatestModelRolloutData(
  ExternalId(modelRollout.externalModelProviderId),
  ExternalId(modelRollout.externalModelSuiteId),
  ExternalId(modelRollout.externalModelLineId)
)

Previous model rollout should never be updated.

Marco-Premier · 2023-06-26T17:51:00Z

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

That is true @tristanvuong2021 .
Instant rollouts cannot be schedule whenever. They must follow the same logic for rollouts with rollout_period.
@jojijac0b this should become:
val latestModelRolloutData =
  readLatestModelRolloutData(
  ExternalId(modelRollout.externalModelProviderId),
  ExternalId(modelRollout.externalModelSuiteId),
  ExternalId(modelRollout.externalModelLineId)
)
Previous model rollout should never be updated.

@SanjayVas this means that once we merge PR #151 we cannot rely on modelRollout.rolloutPeriodStartTime != modelRollout.rolloutPeriodEndTime anymore. I'd suggest to add a new field in public api instant_rollout_time which is REQUIRED only when timeInterval is not provided. What do you think?

SanjayVas

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jojijac0b and @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

@SanjayVas this means that once we merge PR #151 we cannot rely on modelRollout.rolloutPeriodStartTime != modelRollout.rolloutPeriodEndTime anymore. I'd suggest to add a new field in public api instant_rollout_time which is REQUIRED only when timeInterval is not provided. What do you think?

Yes, a oneof where you either have a rollout period or a rollout time sounds correct.

Note that there's not actually a PR yet for world-federation-of-advertisers/cross-media-measurement-api#151. Rather, I'm stating that world-federation-of-advertisers/cross-media-measurement-api#130 introduced a documentation bug where the documentation does not match the previous meaning/usage of TimeInterval.

…time) ModelRollout's previous rollout to null. This will be treated as regular ModelRollout. Update test to reflect this change.

jojijac0b

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @SanjayVas, and @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Yes, a oneof where you either have a rollout period or a rollout time sounds correct.

Note that there's not actually a PR yet for world-federation-of-advertisers/cross-media-measurement-api#151. Rather, I'm stating that world-federation-of-advertisers/cross-media-measurement-api#130 introduced a documentation bug where the documentation does not match the previous meaning/usage of TimeInterval.

Done. Instant ModelRollouts will now be treated as regular ModelRollouts.

SanjayVas

Reviewed 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)

Marco-Premier

Reviewed 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, jojijac0b wrote…

Done. Instant ModelRollouts will now be treated as regular ModelRollouts.

Ok @SanjayVas . I'd say we can wait for the PR to replace TimeInterval to fix both: logic and documentation.
LMK if you disagree.
thanks

Marco-Premier · 2023-06-27T12:15:13Z

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

Ok @SanjayVas . I'd say we can wait for the PR to replace TimeInterval to fix both: logic and documentation.
LMK if you disagree.
thanks

Otherwise we can simply fix this on a separate PR and introduce the oneof

SanjayVas

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

Otherwise we can simply fix this on a separate PR and introduce the oneof

I think the documentation for TimeInterval should be fixed now, which necessitates the introduction of a oneof for this case. Swapping from TimeInterval to google.type.Interval after that should be trivial.

Marco-Premier

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tristanvuong2021)

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/CreateModelRollout.kt line 76 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I think the documentation for TimeInterval should be fixed now, which necessitates the introduction of a oneof for this case. Swapping from TimeInterval to google.type.Interval after that should be trivial.

Created PR for it: world-federation-of-advertisers/cross-media-measurement-api#154

tristanvuong2021

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jojijac0b)

…tTime after that of previous ModelRollout (#1076) Update query in readModelRolloutData to return the ModelRollout for a given ModelLine with the latest RolloutPeriodStartTime in CreateModelRollout.kt. Update CreateModelRollout.kt to throw ModelRolloutInvalidArgsException if the RolloutPeriodStartTime of the ModelRollout to be created precedes that of the last ModelRollout. Create tests in ModelRolloutServiceTest.kt. Update 'createModelRollout succeeds with multiple model releases' test to adhere to new RolloutPeriodStartTime limitations.

jojijac0b requested review from Marco-Premier, SanjayVas and tristanvuong2021 June 22, 2023 19:47

SanjayVas requested changes Jun 22, 2023

View reviewed changes

jojijac0b commented Jun 23, 2023

View reviewed changes

SanjayVas requested changes Jun 23, 2023

View reviewed changes

Marco-Premier approved these changes Jun 26, 2023

View reviewed changes

Update name of test to reflect that it is testing createModelRollout …

c5a8c66

…with multiple model rollouts. Remove comment in API definition(will create another PR in public API).

jojijac0b commented Jun 26, 2023

View reviewed changes

tristanvuong2021 requested changes Jun 26, 2023

View reviewed changes

SanjayVas requested a review from tristanvuong2021 June 26, 2023 17:34

SanjayVas reviewed Jun 26, 2023

View reviewed changes

Remove logic that sets an instant(rollout start time === rollout end …

9570fd3

…time) ModelRollout's previous rollout to null. This will be treated as regular ModelRollout. Update test to reflect this change.

jojijac0b commented Jun 26, 2023

View reviewed changes

SanjayVas approved these changes Jun 26, 2023

View reviewed changes

Marco-Premier approved these changes Jun 27, 2023

View reviewed changes

SanjayVas approved these changes Jun 27, 2023

View reviewed changes

Marco-Premier approved these changes Jun 28, 2023

View reviewed changes

tristanvuong2021 approved these changes Jul 6, 2023

View reviewed changes

jojijac0b enabled auto-merge (squash) July 7, 2023 14:29

Merge branch 'main' into jojijacob-model-rollout-fix

9b5b4fd

jojijac0b merged commit ed63c3a into main Jul 7, 2023

jojijac0b deleted the jojijacob-model-rollout-fix branch July 7, 2023 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix that only allows creation of ModelRollouts with RolloutPeriodStartTime after that of previous ModelRollout #1076

Fix that only allows creation of ModelRollouts with RolloutPeriodStartTime after that of previous ModelRollout #1076

jojijac0b commented Jun 22, 2023 •

edited

Loading

wfa-reviewable commented Jun 22, 2023

SanjayVas left a comment

jojijac0b left a comment

SanjayVas left a comment

Marco-Premier left a comment

jojijac0b left a comment

tristanvuong2021 left a comment

SanjayVas left a comment

SanjayVas left a comment

Marco-Premier commented Jun 26, 2023

Marco-Premier commented Jun 26, 2023

SanjayVas left a comment

jojijac0b left a comment

SanjayVas left a comment

Marco-Premier left a comment

Marco-Premier commented Jun 27, 2023

SanjayVas left a comment

Marco-Premier left a comment

tristanvuong2021 left a comment

Fix that only allows creation of ModelRollouts with RolloutPeriodStartTime after that of previous ModelRollout #1076

Fix that only allows creation of ModelRollouts with RolloutPeriodStartTime after that of previous ModelRollout #1076

Conversation

jojijac0b commented Jun 22, 2023 • edited Loading

wfa-reviewable commented Jun 22, 2023

SanjayVas left a comment

Choose a reason for hiding this comment

jojijac0b left a comment

Choose a reason for hiding this comment

SanjayVas left a comment

Choose a reason for hiding this comment

Marco-Premier left a comment

Choose a reason for hiding this comment

jojijac0b left a comment

Choose a reason for hiding this comment

tristanvuong2021 left a comment

Choose a reason for hiding this comment

SanjayVas left a comment

Choose a reason for hiding this comment

SanjayVas left a comment

Choose a reason for hiding this comment

Marco-Premier commented Jun 26, 2023

Marco-Premier commented Jun 26, 2023

SanjayVas left a comment

Choose a reason for hiding this comment

jojijac0b left a comment

Choose a reason for hiding this comment

SanjayVas left a comment

Choose a reason for hiding this comment

Marco-Premier left a comment

Choose a reason for hiding this comment

Marco-Premier commented Jun 27, 2023

SanjayVas left a comment

Choose a reason for hiding this comment

Marco-Premier left a comment

Choose a reason for hiding this comment

tristanvuong2021 left a comment

Choose a reason for hiding this comment

jojijac0b commented Jun 22, 2023 •

edited

Loading