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

Create Gaussian noise and ACDP composition rollout doc #1185

Merged
merged 1 commit into from
Sep 1, 2023

Conversation

iverson52000
Copy link
Contributor

@wfa-reviewable
Copy link

This change is Reviewable

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @iverson52000, @kungfucraig, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 3 at r1 (raw file):

# Gaussian Noise and ACDP Composition in PBM

Use Gaussian Differential Privacy(DP) noise for Multi-Party Computation(MPC) and

This doc needs to more clearly split the ACDP and Gaussian noise parts, but note their interaction. The important part is that Gaussian noise is enabled as part of the Kingdom configuration, but ACDP is part of the PBM implementation within an EDP's Requisition fulfillment pipeline.


docs/operations/gaussian-noise-and-acdp.md line 18 at r1 (raw file):

1.  Change `noise_mechanism` to `DISCRETE_GAUSSIAN` in
    [llv2_protocol_config_config.textproto](../../src/main/k8s/testing/secretfiles/llv2_protocol_config_config.textproto)
    in Kingdom for LLV2 based MPC reach and frequency measurement

This is just linking to the example config used by our dev environment. You need to instead mention where this would be in their own Kingdom.

Suggestion:

1.  Change `noise_mechanism` to `DISCRETE_GAUSSIAN` in the Kingdom's `Llv2ProtocolConfigConfig`.

    In the `dev` configuration, this is read from the `llv2_protocol_config_config.textproto` file in the `certs-and-configs` Kubernetes secret.

docs/operations/gaussian-noise-and-acdp.md line 21 at r1 (raw file):

2.  Change `noise_mechanism` to `DISCRETE_GAUSSIAN` in
    [ro_llv2_protocol_config_config.textproto](../../src/main/k8s/testing/secretfiles/ro_llv2_protocol_config_config.textproto)
    in Kingdom for LLV2 based MPC reach only measurement

Don't we also need a step to ensure that CONTINUOUS_GAUSSIAN is enabled for the Direct protocol? Even if it's set in our defaults, you need to make sure that the Kingdom operator didn't change the flag to be CONTINUOUS_LAPLACE only.


docs/operations/gaussian-noise-and-acdp.md line 22 at r1 (raw file):

    [ro_llv2_protocol_config_config.textproto](../../src/main/k8s/testing/secretfiles/ro_llv2_protocol_config_config.textproto)
    in Kingdom for LLV2 based MPC reach only measurement
3.  Change `compositionMechanism` flag to `ACDP` in

This is not actually a required step for enabling this feature. It's just something you need to do in order to run correctness tests after enabling it.

More broadly, EDP simulators are not part of a regular production deployment.


docs/operations/gaussian-noise-and-acdp.md line 28 at r1 (raw file):

    provided in
    `src/main/kotlin/org/wfanet/measurement/eventdataprovider/noiser/GaussianNoiser.kt`

The final step here should be EDPs switching their PBMs to using ACDP, right? I assume there's no migration, meaning that this effectively resets the ledger for everyone?

Note that this means there will be a gap between when we're using Gaussian for LLv2 and when the EDP is using ACDP for PBM.


docs/operations/gaussian-noise-and-acdp.md line 59 at r1 (raw file):

For MPC Gaussian noise, the config files in the above Rollout Steps are part of
the testing secrets and are only used by the Halo dev environment. The Kingdom

This doc should only cover the parts that the Kingdom operator and EDP integrators need to do. We control the dev environment and therefore don't need a rollout doc there. Dev rollout happens automatically on release.

Code quote:

For MPC Gaussian noise, the config files in the above Rollout Steps are part of
the testing secrets and are only used by the Halo dev environment. The Kingdom

docs/operations/gaussian-noise-and-acdp.md line 72 at r1 (raw file):

## Emergency Rollback

The Gaussian DP noise and PBM ACDP composition features are designed with

Aren't the ledgers for ACDP and any existing PBM not compatible (see above comment)? If so, rolling back on the EDP side has the effect of dropping all the privacy charges.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @iverson52000, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 0 at r1 (raw file):
Having some file naming convention like:

rollout-.md I think would make sense.

Even doing something like rollout-2023-08-.md could be helpful.

@SanjayVas WDYT?

Alternatively a subdirectory of operations called "rollout" might help separate things that are essentially one-of from things that are more on going operations tasks.


docs/operations/gaussian-noise-and-acdp.md line 3 at r1 (raw file):

# Gaussian Noise and ACDP Composition in PBM

Use Gaussian Differential Privacy(DP) noise for Multi-Party Computation(MPC) and

This guide describes how to enable Gaussian Differential Privacy noise for multi-party computations (MPCs) and ...


docs/operations/gaussian-noise-and-acdp.md line 13 at r1 (raw file):

## Rollout Steps

You should mention that before starting the Kingdom rollout of gaussian noise that the first rollout step is to ensure that all EDP PBMs in the deployment support gaussian noise, and ideally the full ACDP implementation (or if not that they understand the impact of this).


docs/operations/gaussian-noise-and-acdp.md line 28 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The final step here should be EDPs switching their PBMs to using ACDP, right? I assume there's no migration, meaning that this effectively resets the ledger for everyone?

Note that this means there will be a gap between when we're using Gaussian for LLv2 and when the EDP is using ACDP for PBM.

That's the first step. :)


docs/operations/gaussian-noise-and-acdp.md line 31 at r1 (raw file):

## Verification

When PBM is using ACDP compsoition and `noise_mechanism` passed in

Please use a complete sentence.

Copy link
Contributor Author

@iverson52000 iverson52000 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 11 unresolved discussions (waiting on @kungfucraig, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 3 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

This guide describes how to enable Gaussian Differential Privacy noise for multi-party computations (MPCs) and ...

Done.


docs/operations/gaussian-noise-and-acdp.md line 13 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

You should mention that before starting the Kingdom rollout of gaussian noise that the first rollout step is to ensure that all EDP PBMs in the deployment support gaussian noise, and ideally the full ACDP implementation (or if not that they understand the impact of this).

Done.


docs/operations/gaussian-noise-and-acdp.md line 18 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is just linking to the example config used by our dev environment. You need to instead mention where this would be in their own Kingdom.

Done.


docs/operations/gaussian-noise-and-acdp.md line 21 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Don't we also need a step to ensure that CONTINUOUS_GAUSSIAN is enabled for the Direct protocol? Even if it's set in our defaults, you need to make sure that the Kingdom operator didn't change the flag to be CONTINUOUS_LAPLACE only.

Done.


docs/operations/gaussian-noise-and-acdp.md line 22 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is not actually a required step for enabling this feature. It's just something you need to do in order to run correctness tests after enabling it.

More broadly, EDP simulators are not part of a regular production deployment.

Done. Tried to rephrase it to clarify.


docs/operations/gaussian-noise-and-acdp.md line 31 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Please use a complete sentence.

Done.


docs/operations/gaussian-noise-and-acdp.md line 72 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Aren't the ledgers for ACDP and any existing PBM not compatible (see above comment)? If so, rolling back on the EDP side has the effect of dropping all the privacy charges.

Done.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 11 unresolved discussions (waiting on @iverson52000, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 7 at r1 (raw file):

composition mechanism in Privacy Budget Manager(PBM) to improve privacy
accounting. Gaussian noise and ACDP composition can support more than 2x queries
with the same privacy budget compared to Laplace noise and Advanced composition

Advanced need not be capitalized.


docs/operations/gaussian-noise-and-acdp.md line 13 at r1 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Done.

I don't see the update, but if you add the following sentence right here, I'd be happy. :)

"Before enabling Gaussian noise in the Kingdom is is important to ensure that all EDPs who implement a PBM are capable of accounting for Gaussian noise. The should also have implemented ACDP, but if they have not they should be comfortable with Gaussian noise allowing fewer queries that Laplace noise."

Copy link
Contributor Author

@iverson52000 iverson52000 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 11 unresolved discussions (waiting on @kungfucraig, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 3 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This doc needs to more clearly split the ACDP and Gaussian noise parts, but note their interaction. The important part is that Gaussian noise is enabled as part of the Kingdom configuration, but ACDP is part of the PBM implementation within an EDP's Requisition fulfillment pipeline.

Maybe split up Gaussian noise and ACDP composition into sub-sections under Rollout Steps?


docs/operations/gaussian-noise-and-acdp.md line 7 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Advanced need not be capitalized.

Done.


docs/operations/gaussian-noise-and-acdp.md line 13 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

I don't see the update, but if you add the following sentence right here, I'd be happy. :)

"Before enabling Gaussian noise in the Kingdom is is important to ensure that all EDPs who implement a PBM are capable of accounting for Gaussian noise. The should also have implemented ACDP, but if they have not they should be comfortable with Gaussian noise allowing fewer queries that Laplace noise."

Done. Thanks for the suggestion.


docs/operations/gaussian-noise-and-acdp.md line 28 at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

That's the first step. :)

Yes ACDP composition will write privacy charges to a new table(PrivacyBucketAcdpCharges) which is effectively resetting the ledger. Added the note to clarify it


docs/operations/gaussian-noise-and-acdp.md line 59 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This doc should only cover the parts that the Kingdom operator and EDP integrators need to do. We control the dev environment and therefore don't need a rollout doc there. Dev rollout happens automatically on release.

Got it. How should I rephrase this part? Delete "For MPC Gaussian noise, the config files in the above Rollout Steps are part of
the testing secrets and are only used by the Halo dev environment."?

@SanjayVas SanjayVas requested a review from kungfucraig August 28, 2023 18:02
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 11 unresolved discussions (waiting on @kungfucraig, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line at r1 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Having some file naming convention like:

rollout-.md I think would make sense.

Even doing something like rollout-2023-08-.md could be helpful.

@SanjayVas WDYT?

Alternatively a subdirectory of operations called "rollout" might help separate things that are essentially one-of from things that are more on going operations tasks.

The repo shouldn't have one-off rollout info. It should instead only have docs that relate to the state of the code.

In this case, the doc should just be about the requirements/steps for enabling these features.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @iverson52000, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The repo shouldn't have one-off rollout info. It should instead only have docs that relate to the state of the code.

In this case, the doc should just be about the requirements/steps for enabling these features.

Then we can use a different word instead of rollout, but fundamentally these types of docs are different than basic "how to you set up the system for the first time" types of things, and having them all lumped into one place seems confusing.


docs/operations/gaussian-noise-and-acdp.md line 48 at r3 (raw file):

    table(PrivacyBucketAcdpCharges) which is effectively resetting the ledger.

## Verification

Can we document some ways to verify this in a running system? These all seem to be about code level verification.

Something I can think of: having EDPs confirm that their requisitions have this, confirming that the protocol config in the DB has this, confirming with Duchy operators that they see it coming though.

@SanjayVas SanjayVas requested a review from kungfucraig August 28, 2023 21:02
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @iverson52000, @kungfucraig, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 3 at r1 (raw file):

under Rollout Steps

This guide shouldn't even have a Rollout Steps section. It's really about how a Kingdom operator would go about enabling EDPs to use ACDP for their PBM. They do this by configuring the protocols to use/allow Gaussian noise mechanisms. See the other comment regarding naming.

There's really the following discrete parts for the Kingdom operator:

  • Configuring MPC protocols to use Gaussian noise
    • Configuring R/F LLv2
    • Configuring RO LLv2
  • Allowing the Direct protocol to use Gaussian noise
    • Prerequisite of EDPs reading possible noise mechanisms from ProtocolConfig.

The above are prerequisites to an EDP using ACDP. The EDP additionally needs to actually support and use Gaussian noise if it's allowed for the Direct protocol. Furthermore, the EDP should be using ACDP if Gaussian is used for all protocols.


docs/operations/gaussian-noise-and-acdp.md line 22 at r1 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Done. Tried to rephrase it to clarify.

Th EDP simulator isn't actually a reference implementation but a part of the correctness test infrastructure. It's not actually how any EDP should implement requisition fulfillment, but nonetheless can provide some guidance.

I would clarify this as a note for how to simulate an EDP using ACDP in a test environment.


docs/operations/gaussian-noise-and-acdp.md line 28 at r1 (raw file):

That's the first step. :)

I don't believe that's correct. Gaussian being used for LLv2 and allowed for Direct is a prerequisite for an EDP using ACDP.


docs/operations/gaussian-noise-and-acdp.md line 59 at r1 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Got it. How should I rephrase this part? Delete "For MPC Gaussian noise, the config files in the above Rollout Steps are part of
the testing secrets and are only used by the Halo dev environment."?

Yeah. Basically don't even reference the testing files.


docs/operations/gaussian-noise-and-acdp.md line at r1 (raw file):

Then we can use a different word instead of rollout, but fundamentally these types of docs are different than basic "how to you set up the system for the first time" types of things

I'm not sure what you mean. docs/operations is the directory specifically for operations docs, meaning things you might want to do as part of operating a component. This is specifically for things that you may do after a component is up and running.

To follow the existing naming pattern, maybe this should be enabling-gaussian-noise.md or enabling-acdp-pbm.md.

Copy link
Contributor Author

@iverson52000 iverson52000 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @kungfucraig, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 3 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

under Rollout Steps

This guide shouldn't even have a Rollout Steps section. It's really about how a Kingdom operator would go about enabling EDPs to use ACDP for their PBM. They do this by configuring the protocols to use/allow Gaussian noise mechanisms. See the other comment regarding naming.

There's really the following discrete parts for the Kingdom operator:

  • Configuring MPC protocols to use Gaussian noise
    • Configuring R/F LLv2
    • Configuring RO LLv2
  • Allowing the Direct protocol to use Gaussian noise
    • Prerequisite of EDPs reading possible noise mechanisms from ProtocolConfig.

The above are prerequisites to an EDP using ACDP. The EDP additionally needs to actually support and use Gaussian noise if it's allowed for the Direct protocol. Furthermore, the EDP should be using ACDP if Gaussian is used for all protocols.

Done. Restructure the doc and hope it's more clear now.


docs/operations/gaussian-noise-and-acdp.md line 22 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Th EDP simulator isn't actually a reference implementation but a part of the correctness test infrastructure. It's not actually how any EDP should implement requisition fulfillment, but nonetheless can provide some guidance.

I would clarify this as a note for how to simulate an EDP using ACDP in a test environment.

Done.


docs/operations/gaussian-noise-and-acdp.md line 59 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Yeah. Basically don't even reference the testing files.

Done.


docs/operations/gaussian-noise-and-acdp.md line at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Then we can use a different word instead of rollout, but fundamentally these types of docs are different than basic "how to you set up the system for the first time" types of things

I'm not sure what you mean. docs/operations is the directory specifically for operations docs, meaning things you might want to do as part of operating a component. This is specifically for things that you may do after a component is up and running.

To follow the existing naming pattern, maybe this should be enabling-gaussian-noise.md or enabling-acdp-pbm.md.

Rename file to enabling-gaussian-noise-and-acdp-pbm.md to follow existing naming pattern.


docs/operations/gaussian-noise-and-acdp.md line 48 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

Can we document some ways to verify this in a running system? These all seem to be about code level verification.

Something I can think of: having EDPs confirm that their requisitions have this, confirming that the protocol config in the DB has this, confirming with Duchy operators that they see it coming though.

I checked and see MeasurementDetails in Measurements table has protocol config for each measurement. Should we add that as verification here?

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @iverson52000, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/gaussian-noise-and-acdp.md line 48 at r3 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

I checked and see MeasurementDetails in Measurements table has protocol config for each measurement. Should we add that as verification here?

That sounds good.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r4, all commit messages.
Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @iverson52000, @stevenwarejones, and @uakyol)


docs/operations/updating-retention-policies.md line 1 at r4 (raw file):

# Updating Retention Policies

nit: I don't think this file was otherwise edited for this PR. Avoid unrelated formatting changes.


docs/operations/updating-retention-policies.md line 100 at r4 (raw file):

### Human-Readable Duration

Human-readable format consists of a sequence elements each consisting of a

The formatter appears to have broken this.


docs/operations/enabling-gaussian-noise-and-acdp-pbm.md line 24 at r4 (raw file):

Gaussian noise to their direct measurements. The Gaussian noiser implementation
is provided in
`src/main/kotlin/org/wfanet/measurement/eventdataprovider/noiser/GaussianNoiser.kt`

nit: Use a relative link.

Suggestion:

[`GaussianNoiser.kt`](../../src/main/kotlin/org/wfanet/measurement/eventdataprovider/noiser/GaussianNoiser.kt)

docs/operations/enabling-gaussian-noise-and-acdp-pbm.md line 87 at r4 (raw file):

Kingdom Operator and EDP with PBM

I think you can drop this whole section. It's all covered above.


docs/operations/gaussian-noise-and-acdp.md line 22 at r1 (raw file):

Previously, iverson52000 (Albert Hsu) wrote…

Done.

This still not quite making it clear that this is not a normal step, but rather something just for test environments. It also references code rather than the command-line option. I was thinking something like:

1.  When a Kingdom is configured to use discrete Gaussian noise for MPC
    requisitions, EDPs with PBM should update their requisition fulfillment
    pipelines to use ACDP composition.

    Note: If you have a testing environment with EDP simulators, you can
    update those simulators to use ACDP by setting the value of the
    `--composition-mechanism` option to `ACDP`.

Copy link
Contributor Author

@iverson52000 iverson52000 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @kungfucraig, @SanjayVas, @stevenwarejones, and @uakyol)


docs/operations/updating-retention-policies.md line 100 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

The formatter appears to have broken this.

Done. Oops. Not sure how this file is touched


docs/operations/enabling-gaussian-noise-and-acdp-pbm.md line 87 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I think you can drop this whole section. It's all covered above.

Done.


docs/operations/gaussian-noise-and-acdp.md line 22 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This still not quite making it clear that this is not a normal step, but rather something just for test environments. It also references code rather than the command-line option. I was thinking something like:

1.  When a Kingdom is configured to use discrete Gaussian noise for MPC
    requisitions, EDPs with PBM should update their requisition fulfillment
    pipelines to use ACDP composition.

    Note: If you have a testing environment with EDP simulators, you can
    update those simulators to use ACDP by setting the value of the
    `--composition-mechanism` option to `ACDP`.

Done. Thanks for the suggestion


docs/operations/gaussian-noise-and-acdp.md line 48 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

That sounds good.

Done.

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SanjayVas, @stevenwarejones, and @uakyol)

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SanjayVas, @stevenwarejones, and @uakyol)

Copy link
Member

@kungfucraig kungfucraig left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SanjayVas, @stevenwarejones, and @uakyol)

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones and @uakyol)

Copy link
Contributor

@uakyol uakyol left a comment

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants