Proposal on porting PAssert away from aggregators

[Pablo Estrada <pa...@google.com.INVALID>]

Hello all,

BEAM-775 aims to remove Aggregators from the Java SDK. One of the blocking
issues is BEAM-1148, porting PAssert away from using them. Let me summarize
the issues that we should consider; and then propose a solution that seems
reasonable:

1. We want the test Pipeline run to fail-fast in the case of a failed
assertion.
2. We want the test Pipeline run to fail if not all of the assertions were
executed. This catches bugs that would otherwise be hidden.
3. We want to provide a simple default implementation for PAssert. Metrics
are not supported by all runners and when they are both of
committed/attempted values may not be available.ut they seem like a
reasonable default mechanism to count success/failure of assertions.

With this in mind, I propose the following:
1. The mechanism to count/check assertions will be a composite PTransform
that receives the results from the PAssert transforms. Test runners will be
able to override the implementiation of this PTransform.

* The current asserting DoFns in PAssert will go from being DoFn<ActualT,
Void> to be DoFn<ActualT, SuccesOrFailure>, and pass the output
SuccessOrFailure objects downstream to the checking
PTransform<SuccessOrFailure, Void>.
* This default PTransform<SuccessOrFailure, Void> will use metrics to count
successes, and fail immediately on failures (after incrementing the failure
counter).

2. A test runner can override the default implementations of this
PTransform using the graph surgery API.

3. Every TestXRunner implements its own PAssert verification, so we may
need to change that individually - though these changes are not likely to
be very significant for the existing runners.

What are your thoughts?
If everyone is okay with this proposed idea, I will go ahead and implement
it.
Best
-P.

Re: Proposal on porting PAssert away from aggregators

[Kenneth Knowles <kl...@google.com.INVALID>]

On Thu, Mar 30, 2017 at 3:42 PM, Ben Chambers <bc...@google.com.invalid>
wrote:

> When you say instance do you mean a different named PTransform class? Or
> just a separate instance of whatever "HandleAssertResult" PTransform is
> introduced? If the latter, I believe that makes sense.


The latter. One class that "counts to one", and one new CountToOneSuccess
after each assertion. It would do the metrics thing (which may be silently
dropped by the runner) and also fail if it never sees a Success.


> It also addresses
> the problems with committed vs. attempted metrics. Specifically:
>
> Since we want to know that each assertion has run, we'd like to do
> something like "Number of successful assertions" = N. But, in the presence
> of retries, the number of successful assertions may be over counted. But,
> if we have a separate transform for each assertion, then we can say "for
> each transform, the number of successful assertions should be >= 1", which
> ensures that every assert has succeeded. This will work with any metric
> implementation that may overcount but never (or sufficiently rarely)
> undercount.
>
> On Thu, Mar 30, 2017 at 3:39 PM Kenneth Knowles <kl...@google.com.invalid>
> wrote:
>
> > This sounds pretty good to me. I really like emphasizing the ability of a
> > runner to go ahead and use pipeline surgery to put in their own
> > verification transform.
> >
> > On Thu, Mar 30, 2017 at 3:12 PM, Pablo Estrada
> <pabloem@google.com.invalid
> > >
> > wrote:
> >
> > > * The current asserting DoFns in PAssert will go from being
> DoFn<ActualT,
> > > Void> to be DoFn<ActualT, SuccesOrFailure>, and pass the output
> > > SuccessOrFailure objects downstream to the checking
> > > PTransform<SuccessOrFailure, Void>.
> > > * This default PTransform<SuccessOrFailure, Void> will use metrics to
> > count
> > > successes, and fail immediately on failures (after incrementing the
> > failure
> > > counter).
> > >
> >
> > Could it possibly be useful to have a separate instance of a
> > PTransform<SuccessOrFailure> for each assertion? It might be able to
> proxy
> > for counting, for runners that don't yet have even rudimentary metrics.
> The
> > transform could set up triggering to fire just once and fail if it has
> not
> > received one Success. The runner might still just not execute that
> > transform, but we could set it up so that is extremely unlikely, and
> anyhow
> > it is better than no coverage. I could be missing something about the
> > implementation issues here, though.
> >
> > Kenn
> >
>

Re: Proposal on porting PAssert away from aggregators

[Ben Chambers <bc...@google.com.INVALID>]

When you say instance do you mean a different named PTransform class? Or
just a separate instance of whatever "HandleAssertResult" PTransform is
introduced? If the latter, I believe that makes sense. It also addresses
the problems with committed vs. attempted metrics. Specifically:

Since we want to know that each assertion has run, we'd like to do
something like "Number of successful assertions" = N. But, in the presence
of retries, the number of successful assertions may be over counted. But,
if we have a separate transform for each assertion, then we can say "for
each transform, the number of successful assertions should be >= 1", which
ensures that every assert has succeeded. This will work with any metric
implementation that may overcount but never (or sufficiently rarely)
undercount.

On Thu, Mar 30, 2017 at 3:39 PM Kenneth Knowles <kl...@google.com.invalid>
wrote:

> This sounds pretty good to me. I really like emphasizing the ability of a
> runner to go ahead and use pipeline surgery to put in their own
> verification transform.
>
> On Thu, Mar 30, 2017 at 3:12 PM, Pablo Estrada <pabloem@google.com.invalid
> >
> wrote:
>
> > * The current asserting DoFns in PAssert will go from being DoFn<ActualT,
> > Void> to be DoFn<ActualT, SuccesOrFailure>, and pass the output
> > SuccessOrFailure objects downstream to the checking
> > PTransform<SuccessOrFailure, Void>.
> > * This default PTransform<SuccessOrFailure, Void> will use metrics to
> count
> > successes, and fail immediately on failures (after incrementing the
> failure
> > counter).
> >
>
> Could it possibly be useful to have a separate instance of a
> PTransform<SuccessOrFailure> for each assertion? It might be able to proxy
> for counting, for runners that don't yet have even rudimentary metrics. The
> transform could set up triggering to fire just once and fail if it has not
> received one Success. The runner might still just not execute that
> transform, but we could set it up so that is extremely unlikely, and anyhow
> it is better than no coverage. I could be missing something about the
> implementation issues here, though.
>
> Kenn
>

Re: Proposal on porting PAssert away from aggregators

[Kenneth Knowles <kl...@google.com.INVALID>]

This sounds pretty good to me. I really like emphasizing the ability of a
runner to go ahead and use pipeline surgery to put in their own
verification transform.

On Thu, Mar 30, 2017 at 3:12 PM, Pablo Estrada <pa...@google.com.invalid>
wrote:

> * The current asserting DoFns in PAssert will go from being DoFn<ActualT,
> Void> to be DoFn<ActualT, SuccesOrFailure>, and pass the output
> SuccessOrFailure objects downstream to the checking
> PTransform<SuccessOrFailure, Void>.
> * This default PTransform<SuccessOrFailure, Void> will use metrics to count
> successes, and fail immediately on failures (after incrementing the failure
> counter).
>

Could it possibly be useful to have a separate instance of a
PTransform<SuccessOrFailure> for each assertion? It might be able to proxy
for counting, for runners that don't yet have even rudimentary metrics. The
transform could set up triggering to fire just once and fail if it has not
received one Success. The runner might still just not execute that
transform, but we could set it up so that is extremely unlikely, and anyhow
it is better than no coverage. I could be missing something about the
implementation issues here, though.

Kenn

Re: Proposal on porting PAssert away from aggregators

[Dan Halperin <dh...@google.com.INVALID>]

Yeah, this sounds really nice. It also seems to let runners do "whatever
they want" -- write to files, switch to some magic but small-scale state,
etc.

On Thu, Mar 30, 2017 at 10:50 PM, Jean-Baptiste Onofré <jb...@nanthrax.net>
wrote:

> Hi Pablo,
>
> it sounds a good idea !
>
> Regards
> JB
>
>
> On 03/31/2017 12:12 AM, Pablo Estrada wrote:
>
>> Hello all,
>>
>> BEAM-775 aims to remove Aggregators from the Java SDK. One of the blocking
>> issues is BEAM-1148, porting PAssert away from using them. Let me
>> summarize
>> the issues that we should consider; and then propose a solution that seems
>> reasonable:
>>
>> 1. We want the test Pipeline run to fail-fast in the case of a failed
>> assertion.
>> 2. We want the test Pipeline run to fail if not all of the assertions were
>> executed. This catches bugs that would otherwise be hidden.
>> 3. We want to provide a simple default implementation for PAssert. Metrics
>> are not supported by all runners and when they are both of
>> committed/attempted values may not be available.ut they seem like a
>> reasonable default mechanism to count success/failure of assertions.
>>
>> With this in mind, I propose the following:
>> 1. The mechanism to count/check assertions will be a composite PTransform
>> that receives the results from the PAssert transforms. Test runners will
>> be
>> able to override the implementiation of this PTransform.
>>
>> * The current asserting DoFns in PAssert will go from being DoFn<ActualT,
>> Void> to be DoFn<ActualT, SuccesOrFailure>, and pass the output
>> SuccessOrFailure objects downstream to the checking
>> PTransform<SuccessOrFailure, Void>.
>> * This default PTransform<SuccessOrFailure, Void> will use metrics to
>> count
>> successes, and fail immediately on failures (after incrementing the
>> failure
>> counter).
>>
>> 2. A test runner can override the default implementations of this
>> PTransform using the graph surgery API.
>>
>> 3. Every TestXRunner implements its own PAssert verification, so we may
>> need to change that individually - though these changes are not likely to
>> be very significant for the existing runners.
>>
>> What are your thoughts?
>> If everyone is okay with this proposed idea, I will go ahead and implement
>> it.
>> Best
>> -P.
>>
>>
> --
> Jean-Baptiste Onofré
> jbonofre@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com
>

Re: Proposal on porting PAssert away from aggregators

[Jean-Baptiste Onofré <jb...@nanthrax.net>]

Hi Pablo,

it sounds a good idea !

Regards
JB

On 03/31/2017 12:12 AM, Pablo Estrada wrote:
> Hello all,
>
> BEAM-775 aims to remove Aggregators from the Java SDK. One of the blocking
> issues is BEAM-1148, porting PAssert away from using them. Let me summarize
> the issues that we should consider; and then propose a solution that seems
> reasonable:
>
> 1. We want the test Pipeline run to fail-fast in the case of a failed
> assertion.
> 2. We want the test Pipeline run to fail if not all of the assertions were
> executed. This catches bugs that would otherwise be hidden.
> 3. We want to provide a simple default implementation for PAssert. Metrics
> are not supported by all runners and when they are both of
> committed/attempted values may not be available.ut they seem like a
> reasonable default mechanism to count success/failure of assertions.
>
> With this in mind, I propose the following:
> 1. The mechanism to count/check assertions will be a composite PTransform
> that receives the results from the PAssert transforms. Test runners will be
> able to override the implementiation of this PTransform.
>
> * The current asserting DoFns in PAssert will go from being DoFn<ActualT,
> Void> to be DoFn<ActualT, SuccesOrFailure>, and pass the output
> SuccessOrFailure objects downstream to the checking
> PTransform<SuccessOrFailure, Void>.
> * This default PTransform<SuccessOrFailure, Void> will use metrics to count
> successes, and fail immediately on failures (after incrementing the failure
> counter).
>
> 2. A test runner can override the default implementations of this
> PTransform using the graph surgery API.
>
> 3. Every TestXRunner implements its own PAssert verification, so we may
> need to change that individually - though these changes are not likely to
> be very significant for the existing runners.
>
> What are your thoughts?
> If everyone is okay with this proposed idea, I will go ahead and implement
> it.
> Best
> -P.
>

-- 
Jean-Baptiste Onofr�
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com