refactor: Graduate Report post-processing from experimental

[ple13]

No description provided.

[wfa-reviewable]

This change is 

[SanjayVas]

Reviewed 37 of 37 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kungfucraig and @ple13)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/BUILD.bazel line 18 at r1 (raw file):

        "//src/main/proto/wfa/measurement/reporting/postprocessing/v2alpha:report_summary_kt_jvm_proto",
        "//src/main/proto/wfa/measurement/reporting/v2alpha:report_kt_jvm_proto",
        "@maven//:com_google_protobuf_protobuf_java_util",

Use alias from //imports for all Maven deps. Depending on which Bazel module added this dep, this may be under @rules_kotlin_jvm//imports or @wfa_common_jvm//imports

Code quote:

"@maven//:com_google_protobuf_protobuf_java_util",

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 63 at r1 (raw file):

  }

  fun getSetOperation(tag: String): String {

This can be done in a later PR, but can we split off anything that references Origin-specific tags (i.e. anything in this file that references tags at all) to their own package? All Origin-specific code in this repo is temporary and should eventually be deleted.

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessorImpl.kt line 37 at r1 (raw file):

 * returns the a [Report] of which all measurements are consistent.
 */
class ReportProcessorImpl : ReportProcessor {

nit: This can still be a singleton object while implementing the interface.

[ple13]

Reviewable status: 32 of 37 files reviewed, 2 unresolved discussions (waiting on @kungfucraig and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/BUILD.bazel line 18 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use alias from //imports for all Maven deps. Depending on which Bazel module added this dep, this may be under @rules_kotlin_jvm//imports or @wfa_common_jvm//imports

Done. It turns out that we don't need this dependency at all.

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 63 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This can be done in a later PR, but can we split off anything that references Origin-specific tags (i.e. anything in this file that references tags at all) to their own package? All Origin-specific code in this repo is temporary and should eventually be deleted.

Done. I've added TODOs for these functions.

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessorImpl.kt line 37 at r1 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit: This can still be a singleton object while implementing the interface.

Done.

[SanjayVas]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kungfucraig and @ple13)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 50 at r2 (raw file):

  }

  // TODO(@ple13): Move this function to a separate package that handles the tags.

nit

Suggestion:

Origin-specific package

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessorImpl.kt line 36 at r2 (raw file):

 * returns the a [Report] of which all measurements are consistent.
 */
object ReportProcessorImpl : ReportProcessor {

I should have been more clear: follow the pattern of kotlin.Random. There the interface as a companion object named Default which provides the default impl.

[kungfucraig]

Reviewed 32 of 37 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13)

[ple13]

Reviewable status: 33 of 38 files reviewed, 1 unresolved discussion (waiting on @kungfucraig and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 50 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

nit

Done.

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessorImpl.kt line 36 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I should have been more clear: follow the pattern of kotlin.Random. There the interface as a companion object named Default which provides the default impl.

Done.

[SanjayVas]

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

[stevenwarejones]

Reviewed 31 of 37 files at r1, 2 of 5 files at r2, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessor.kt line 93 at r3 (raw file):

      // TODO(bazelbuild/bazel#17629): Execute the Python zip directly once this bug is fixed.
      val processBuilder = ProcessBuilder("python3", tempFile.toPath().toString())

what do you think about using this workaround instead?

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 50 at r3 (raw file):

  }

  // TODO(@ple13): Move this function to a separate Origin-specific package.

is there a doc somewhere on how we want this to look long-term?

[kungfucraig]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ple13 and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 50 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

is there a doc somewhere on how we want this to look long-term?

The noise correction code will be part of the new MC API. As part of Phase II we'll have a doc that answers your question.

We should make it a goal to delete or relocate everything in this PR by the end of 2025.

[ple13]

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

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessor.kt line 93 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

what do you think about using this workaround instead?

The workaround solves the problem. @SanjayVas WDYT about using the workaround?

[SanjayVas]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessor.kt line 93 at r3 (raw file):

what do you think about using this workaround instead?

Sorry, which workaround do you mean? Do you mean the workaround that's already in the code to call python3? That's fine until the bug in the TODO is fixed, after which we should execute the pyzip itself.

If it's some other workaround, perhaps there's a code suggestion that's not showing up for me?

[ple13]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessor.kt line 93 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

what do you think about using this workaround instead?

Sorry, which workaround do you mean? Do you mean the workaround that's already in the code to call python3? That's fine until the bug in the TODO is fixed, after which we should execute the pyzip itself.

If it's some other workaround, perhaps there's a code suggestion that's not showing up for me?

We are talking about this workaround bazelbuild/bazel#17629 (comment).

[SanjayVas]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessor.kt line 93 at r3 (raw file):

Previously, ple13 (Phi) wrote…

We are talking about this workaround bazelbuild/bazel#17629 (comment).

Oh, that one is messy and may break when the bug is fixed. This workaround we have should continue to work even after the bug is fixed, and is easier to clean up.

[ple13]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportProcessor.kt line 93 at r3 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Oh, that one is messy and may break when the bug is fixed. This workaround we have should continue to work even after the bug is fixed, and is easier to clean up.

Thanks @SanjayVas. It makes more sense to me to keep it as it is now.

[stevenwarejones]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kungfucraig and @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 50 at r3 (raw file):

Previously, kungfucraig (Craig Wright) wrote…

The noise correction code will be part of the new MC API. As part of Phase II we'll have a doc that answers your question.

We should make it a goal to delete or relocate everything in this PR by the end of 2025.

can you add some tests for these functions with example tags?

[SanjayVas]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/reporting/postprocessing/v2alpha/ReportConversion.kt line 50 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

can you add some tests for these functions with example tags?

I believe this is covered by existing tests. The sample tags are in the test data JSON Reports.

[stevenwarejones]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kungfucraig and @SanjayVas)