You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Maximilian Michels <mx...@apache.org> on 2018/09/21 13:40:16 UTC
Python PreCommit broken
Hi,
The Python PreCommit tests are currently broken. Is anybody looking into
this?
Example:
https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
JIRA: https://issues.apache.org/jira/browse/BEAM-5458
I'm sure this was just an accident. No big deal but let's make sure
PreCommit passes when merging. A broken PreCommit means that we can't
merge any other changes with confidence.
Thanks,
Max
Re: Python PreCommit broken
Posted by Charles Chen <cc...@google.com>.
I can speak to the breakage from https://github.com/apache/beam/pull/6151:
here, the root cause was that an interface in an unmodified file was
refactored in the interim.
On Fri, Sep 21, 2018 at 2:26 PM Ryan Williams <ry...@runsascoded.com> wrote:
>
> On Fri, Sep 21, 2018 at 5:08 PM Robert Burke <ro...@frantil.com> wrote:
>
>> The issue is time from commit to merge, and without manual intervention,
>> commits from other PRs aren't accounted for, if there's a lag between LGTM
>> and merge.
>>
>
> it's not between LGTM and merge, it's between [last commit to the PR's
> branch] and merge, right?
>
>
>>
>> On Fri, Sep 21, 2018, 1:52 PM Ahmet Altay <al...@google.com> wrote:
>>
>>> I will suggest a rollback in this case, and in general as a good
>>> practice to unblock people.
>>>
>>> On Fri, Sep 21, 2018 at 1:02 PM, Charles Chen <cc...@google.com> wrote:
>>>
>>>> Relatedly, https://github.com/apache/beam/pull/6151 also recently
>>>> broke the build (
>>>> https://builds.apache.org/view/A-D/view/Beam/job/beam_PostCommit_Java_GradleBuild/1503/console)
>>>> because the Precommits were very out of date when merged.
>>>>
>>>
>>> Does not github change the test signals from green to yellow when new
>>> commits are added?
>>>
>>
> it does this if you add commits to your PR's branch, not if commits happen
> upstream.
>
> in the latter case, github only checks whether your PR can be merged
> cleanly (i.e. with no merge-conflicts) according to some line-diffing
> heuristic (presumably the same as git uses), but not whether the tests
> would still pass post-merge.
>
> the unwritten assumption is that a non-conflicting merge with upstream
> won't break any tests, but obviously there can be false-positives; thanks
> for documenting those cases, Valentyn and Charles.
>
> it would be nice to run precommits on potential post-merge commits, but
> i'd guess that our precommits take longer than the average time between
> upstream commits, so it would be hard to get anything merged that way!
>
> we'd need to get smarter about knowing exactly which tests need re-running
> when new commits happen upstream, and only re-run those tests, which we're
> pretty far from today.
>
> so, the possibility of this kind of breakage is always present,
> unfortunately.
>
> i would be curious to see exactly how the line-diffing heuristic was
> fooled in these cases. an easy way to construct such a case is to have one
> side rename a variable, and another side add a new reference to the
> variable's old name, in a newly-created file; each side can touch a
> disjoint set of files and have all its tests pass in isolation, and
> git{,hub} will let you merge them, but the tests will fail post-merge.
>
>
>>
>>>
>>>
>>>>
>>>> On Fri, Sep 21, 2018 at 12:50 PM Valentyn Tymofieiev <
>>>> valentyn@google.com> wrote:
>>>>
>>>>> The change https://github.com/apache/beam/pull/6424 was not deemed
>>>>> particularly risky, and it's purpose was adding more tests to precommit
>>>>> test suite.
>>>>> There was a green Precommit signal on Jenkins, and I believe
>>>>> Postcommit test suite (at the same time) wouldn't catch this.
>>>>>
>>>>> The reason the breakage was introduced is that
>>>>> https://github.com/apache/beam/commit/7689f12db5 was committed after
>>>>> the PR 6424 was reviewed, but before it was merged. A combination of both
>>>>> introduced the breakage.
>>>>>
>>>>> Had we re-run the tests closer to the merge, we should have caught
>>>>> this. Can we automatically re-run precommits tests at merge time, when
>>>>> some of the files a PR is touching have changed since last precommit run?
>>>>>
>>>>> I suggest we proceed with https://github.com/apache/beam/pull/6464 or
>>>>> revert https://github.com/apache/beam/pull/6424 in the mean time,
>>>>> while we are iterating on the fix.
>>>>>
>>>>> On Fri, Sep 21, 2018 at 11:41 AM Charles Chen <cc...@google.com> wrote:
>>>>>
>>>>>> Do we happen to know the root cause for why this wasn't caught during
>>>>>> review / precommit?
>>>>>>
>>>>>> In the future, can we run manually run postcommits for risky changes
>>>>>> like these? That is, trigger it by commenting "Run Python PostCommit"?
>>>>>>
>>>>>> On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pa...@google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Robbe has prepared a better fix on
>>>>>>> https://github.com/apache/beam/pull/6465
>>>>>>> Hopefully that'll be the last of this breakage : )
>>>>>>> -P.
>>>>>>>
>>>>>>> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <
>>>>>>> jb@nanthrax.net> wrote:
>>>>>>>
>>>>>>>> By the way, it fails for me on my machine as well.
>>>>>>>>
>>>>>>>> Regards
>>>>>>>> JB
>>>>>>>>
>>>>>>>> On 21/09/2018 18:10, Pablo Estrada wrote:
>>>>>>>> > I investigated. This failure comes from the activation of
>>>>>>>> > apache_beam.pipeline_test in Python 3 unit tests.
>>>>>>>> >
>>>>>>>> > I have https://github.com/apache/beam/pull/6464 out to fix this.
>>>>>>>> >
>>>>>>>> > In any case, it's very exciting that we have some unit tests
>>>>>>>> running on
>>>>>>>> > Py3 : )
>>>>>>>> > Best
>>>>>>>> > -P.
>>>>>>>> >
>>>>>>>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <
>>>>>>>> mxm@apache.org
>>>>>>>> > <ma...@apache.org>> wrote:
>>>>>>>> >
>>>>>>>> > Hi,
>>>>>>>> >
>>>>>>>> > The Python PreCommit tests are currently broken. Is anybody
>>>>>>>> looking
>>>>>>>> > into
>>>>>>>> > this?
>>>>>>>> >
>>>>>>>> > Example:
>>>>>>>> >
>>>>>>>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
>>>>>>>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>>>>>>>> >
>>>>>>>> > I'm sure this was just an accident. No big deal but let's
>>>>>>>> make sure
>>>>>>>> > PreCommit passes when merging. A broken PreCommit means that
>>>>>>>> we can't
>>>>>>>> > merge any other changes with confidence.
>>>>>>>> >
>>>>>>>> > Thanks,
>>>>>>>> > Max
>>>>>>>> >
>>>>>>>>
>>>>>>>> --
>>>>>>>> Jean-Baptiste Onofré
>>>>>>>> jbonofre@apache.org
>>>>>>>> http://blog.nanthrax.net
>>>>>>>> Talend - http://www.talend.com
>>>>>>>>
>>>>>>>
>>>
Re: Python PreCommit broken
Posted by Ryan Williams <ry...@runsascoded.com>.
On Fri, Sep 21, 2018 at 5:08 PM Robert Burke <ro...@frantil.com> wrote:
> The issue is time from commit to merge, and without manual intervention,
> commits from other PRs aren't accounted for, if there's a lag between LGTM
> and merge.
>
it's not between LGTM and merge, it's between [last commit to the PR's
branch] and merge, right?
>
> On Fri, Sep 21, 2018, 1:52 PM Ahmet Altay <al...@google.com> wrote:
>
>> I will suggest a rollback in this case, and in general as a good practice
>> to unblock people.
>>
>> On Fri, Sep 21, 2018 at 1:02 PM, Charles Chen <cc...@google.com> wrote:
>>
>>> Relatedly, https://github.com/apache/beam/pull/6151 also recently broke
>>> the build (
>>> https://builds.apache.org/view/A-D/view/Beam/job/beam_PostCommit_Java_GradleBuild/1503/console)
>>> because the Precommits were very out of date when merged.
>>>
>>
>> Does not github change the test signals from green to yellow when new
>> commits are added?
>>
>
it does this if you add commits to your PR's branch, not if commits happen
upstream.
in the latter case, github only checks whether your PR can be merged
cleanly (i.e. with no merge-conflicts) according to some line-diffing
heuristic (presumably the same as git uses), but not whether the tests
would still pass post-merge.
the unwritten assumption is that a non-conflicting merge with upstream
won't break any tests, but obviously there can be false-positives; thanks
for documenting those cases, Valentyn and Charles.
it would be nice to run precommits on potential post-merge commits, but i'd
guess that our precommits take longer than the average time between
upstream commits, so it would be hard to get anything merged that way!
we'd need to get smarter about knowing exactly which tests need re-running
when new commits happen upstream, and only re-run those tests, which we're
pretty far from today.
so, the possibility of this kind of breakage is always present,
unfortunately.
i would be curious to see exactly how the line-diffing heuristic was fooled
in these cases. an easy way to construct such a case is to have one side
rename a variable, and another side add a new reference to the variable's
old name, in a newly-created file; each side can touch a disjoint set of
files and have all its tests pass in isolation, and git{,hub} will let you
merge them, but the tests will fail post-merge.
>
>>
>>
>>>
>>> On Fri, Sep 21, 2018 at 12:50 PM Valentyn Tymofieiev <
>>> valentyn@google.com> wrote:
>>>
>>>> The change https://github.com/apache/beam/pull/6424 was not deemed
>>>> particularly risky, and it's purpose was adding more tests to precommit
>>>> test suite.
>>>> There was a green Precommit signal on Jenkins, and I believe Postcommit
>>>> test suite (at the same time) wouldn't catch this.
>>>>
>>>> The reason the breakage was introduced is that
>>>> https://github.com/apache/beam/commit/7689f12db5 was committed after
>>>> the PR 6424 was reviewed, but before it was merged. A combination of both
>>>> introduced the breakage.
>>>>
>>>> Had we re-run the tests closer to the merge, we should have caught
>>>> this. Can we automatically re-run precommits tests at merge time, when
>>>> some of the files a PR is touching have changed since last precommit run?
>>>>
>>>> I suggest we proceed with https://github.com/apache/beam/pull/6464 or
>>>> revert https://github.com/apache/beam/pull/6424 in the mean time,
>>>> while we are iterating on the fix.
>>>>
>>>> On Fri, Sep 21, 2018 at 11:41 AM Charles Chen <cc...@google.com> wrote:
>>>>
>>>>> Do we happen to know the root cause for why this wasn't caught during
>>>>> review / precommit?
>>>>>
>>>>> In the future, can we run manually run postcommits for risky changes
>>>>> like these? That is, trigger it by commenting "Run Python PostCommit"?
>>>>>
>>>>> On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pa...@google.com>
>>>>> wrote:
>>>>>
>>>>>> Robbe has prepared a better fix on
>>>>>> https://github.com/apache/beam/pull/6465
>>>>>> Hopefully that'll be the last of this breakage : )
>>>>>> -P.
>>>>>>
>>>>>> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <jb...@nanthrax.net>
>>>>>> wrote:
>>>>>>
>>>>>>> By the way, it fails for me on my machine as well.
>>>>>>>
>>>>>>> Regards
>>>>>>> JB
>>>>>>>
>>>>>>> On 21/09/2018 18:10, Pablo Estrada wrote:
>>>>>>> > I investigated. This failure comes from the activation of
>>>>>>> > apache_beam.pipeline_test in Python 3 unit tests.
>>>>>>> >
>>>>>>> > I have https://github.com/apache/beam/pull/6464 out to fix this.
>>>>>>> >
>>>>>>> > In any case, it's very exciting that we have some unit tests
>>>>>>> running on
>>>>>>> > Py3 : )
>>>>>>> > Best
>>>>>>> > -P.
>>>>>>> >
>>>>>>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
>>>>>>> > <ma...@apache.org>> wrote:
>>>>>>> >
>>>>>>> > Hi,
>>>>>>> >
>>>>>>> > The Python PreCommit tests are currently broken. Is anybody
>>>>>>> looking
>>>>>>> > into
>>>>>>> > this?
>>>>>>> >
>>>>>>> > Example:
>>>>>>> >
>>>>>>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
>>>>>>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>>>>>>> >
>>>>>>> > I'm sure this was just an accident. No big deal but let's make
>>>>>>> sure
>>>>>>> > PreCommit passes when merging. A broken PreCommit means that
>>>>>>> we can't
>>>>>>> > merge any other changes with confidence.
>>>>>>> >
>>>>>>> > Thanks,
>>>>>>> > Max
>>>>>>> >
>>>>>>>
>>>>>>> --
>>>>>>> Jean-Baptiste Onofré
>>>>>>> jbonofre@apache.org
>>>>>>> http://blog.nanthrax.net
>>>>>>> Talend - http://www.talend.com
>>>>>>>
>>>>>>
>>
Re: Python PreCommit broken
Posted by Robert Burke <ro...@frantil.com>.
The issue is time from commit to merge, and without manual intervention,
commits from other PRs aren't accounted for, if there's a lag between LGTM
and merge.
On Fri, Sep 21, 2018, 1:52 PM Ahmet Altay <al...@google.com> wrote:
> I will suggest a rollback in this case, and in general as a good practice
> to unblock people.
>
> On Fri, Sep 21, 2018 at 1:02 PM, Charles Chen <cc...@google.com> wrote:
>
>> Relatedly, https://github.com/apache/beam/pull/6151 also recently broke
>> the build (
>> https://builds.apache.org/view/A-D/view/Beam/job/beam_PostCommit_Java_GradleBuild/1503/console)
>> because the Precommits were very out of date when merged.
>>
>
> Does not github change the test signals from green to yellow when new
> commits are added?
>
>
>>
>> On Fri, Sep 21, 2018 at 12:50 PM Valentyn Tymofieiev <va...@google.com>
>> wrote:
>>
>>> The change https://github.com/apache/beam/pull/6424 was not deemed
>>> particularly risky, and it's purpose was adding more tests to precommit
>>> test suite.
>>> There was a green Precommit signal on Jenkins, and I believe Postcommit
>>> test suite (at the same time) wouldn't catch this.
>>>
>>> The reason the breakage was introduced is that
>>> https://github.com/apache/beam/commit/7689f12db5 was committed after
>>> the PR 6424 was reviewed, but before it was merged. A combination of both
>>> introduced the breakage.
>>>
>>> Had we re-run the tests closer to the merge, we should have caught this.
>>> Can we automatically re-run precommits tests at merge time, when some of
>>> the files a PR is touching have changed since last precommit run?
>>>
>>> I suggest we proceed with https://github.com/apache/beam/pull/6464 or
>>> revert https://github.com/apache/beam/pull/6424 in the mean time,
>>> while we are iterating on the fix.
>>>
>>> On Fri, Sep 21, 2018 at 11:41 AM Charles Chen <cc...@google.com> wrote:
>>>
>>>> Do we happen to know the root cause for why this wasn't caught during
>>>> review / precommit?
>>>>
>>>> In the future, can we run manually run postcommits for risky changes
>>>> like these? That is, trigger it by commenting "Run Python PostCommit"?
>>>>
>>>> On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pa...@google.com>
>>>> wrote:
>>>>
>>>>> Robbe has prepared a better fix on
>>>>> https://github.com/apache/beam/pull/6465
>>>>> Hopefully that'll be the last of this breakage : )
>>>>> -P.
>>>>>
>>>>> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <jb...@nanthrax.net>
>>>>> wrote:
>>>>>
>>>>>> By the way, it fails for me on my machine as well.
>>>>>>
>>>>>> Regards
>>>>>> JB
>>>>>>
>>>>>> On 21/09/2018 18:10, Pablo Estrada wrote:
>>>>>> > I investigated. This failure comes from the activation of
>>>>>> > apache_beam.pipeline_test in Python 3 unit tests.
>>>>>> >
>>>>>> > I have https://github.com/apache/beam/pull/6464 out to fix this.
>>>>>> >
>>>>>> > In any case, it's very exciting that we have some unit tests
>>>>>> running on
>>>>>> > Py3 : )
>>>>>> > Best
>>>>>> > -P.
>>>>>> >
>>>>>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
>>>>>> > <ma...@apache.org>> wrote:
>>>>>> >
>>>>>> > Hi,
>>>>>> >
>>>>>> > The Python PreCommit tests are currently broken. Is anybody
>>>>>> looking
>>>>>> > into
>>>>>> > this?
>>>>>> >
>>>>>> > Example:
>>>>>> >
>>>>>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
>>>>>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>>>>>> >
>>>>>> > I'm sure this was just an accident. No big deal but let's make
>>>>>> sure
>>>>>> > PreCommit passes when merging. A broken PreCommit means that we
>>>>>> can't
>>>>>> > merge any other changes with confidence.
>>>>>> >
>>>>>> > Thanks,
>>>>>> > Max
>>>>>> >
>>>>>>
>>>>>> --
>>>>>> Jean-Baptiste Onofré
>>>>>> jbonofre@apache.org
>>>>>> http://blog.nanthrax.net
>>>>>> Talend - http://www.talend.com
>>>>>>
>>>>>
>
Re: Python PreCommit broken
Posted by Ahmet Altay <al...@google.com>.
I will suggest a rollback in this case, and in general as a good practice
to unblock people.
On Fri, Sep 21, 2018 at 1:02 PM, Charles Chen <cc...@google.com> wrote:
> Relatedly, https://github.com/apache/beam/pull/6151 also recently broke
> the build (https://builds.apache.org/view/A-D/view/Beam/job/beam_
> PostCommit_Java_GradleBuild/1503/console) because the Precommits were
> very out of date when merged.
>
Does not github change the test signals from green to yellow when new
commits are added?
>
> On Fri, Sep 21, 2018 at 12:50 PM Valentyn Tymofieiev <va...@google.com>
> wrote:
>
>> The change https://github.com/apache/beam/pull/6424 was not deemed
>> particularly risky, and it's purpose was adding more tests to precommit
>> test suite.
>> There was a green Precommit signal on Jenkins, and I believe Postcommit
>> test suite (at the same time) wouldn't catch this.
>>
>> The reason the breakage was introduced is that https://github.com/
>> apache/beam/commit/7689f12db5 was committed after the PR 6424 was
>> reviewed, but before it was merged. A combination of both introduced the
>> breakage.
>>
>> Had we re-run the tests closer to the merge, we should have caught this.
>> Can we automatically re-run precommits tests at merge time, when some of
>> the files a PR is touching have changed since last precommit run?
>>
>> I suggest we proceed with https://github.com/apache/beam/pull/6464 or
>> revert https://github.com/apache/beam/pull/6424 in the mean time, while
>> we are iterating on the fix.
>>
>> On Fri, Sep 21, 2018 at 11:41 AM Charles Chen <cc...@google.com> wrote:
>>
>>> Do we happen to know the root cause for why this wasn't caught during
>>> review / precommit?
>>>
>>> In the future, can we run manually run postcommits for risky changes
>>> like these? That is, trigger it by commenting "Run Python PostCommit"?
>>>
>>> On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pa...@google.com>
>>> wrote:
>>>
>>>> Robbe has prepared a better fix on https://github.com/apache/
>>>> beam/pull/6465
>>>> Hopefully that'll be the last of this breakage : )
>>>> -P.
>>>>
>>>> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <jb...@nanthrax.net>
>>>> wrote:
>>>>
>>>>> By the way, it fails for me on my machine as well.
>>>>>
>>>>> Regards
>>>>> JB
>>>>>
>>>>> On 21/09/2018 18:10, Pablo Estrada wrote:
>>>>> > I investigated. This failure comes from the activation of
>>>>> > apache_beam.pipeline_test in Python 3 unit tests.
>>>>> >
>>>>> > I have https://github.com/apache/beam/pull/6464 out to fix this.
>>>>> >
>>>>> > In any case, it's very exciting that we have some unit tests running
>>>>> on
>>>>> > Py3 : )
>>>>> > Best
>>>>> > -P.
>>>>> >
>>>>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
>>>>> > <ma...@apache.org>> wrote:
>>>>> >
>>>>> > Hi,
>>>>> >
>>>>> > The Python PreCommit tests are currently broken. Is anybody
>>>>> looking
>>>>> > into
>>>>> > this?
>>>>> >
>>>>> > Example:
>>>>> > https://builds.apache.org/job/beam_PreCommit_Python_
>>>>> Commit/1308/#showFailuresLink
>>>>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>>>>> >
>>>>> > I'm sure this was just an accident. No big deal but let's make
>>>>> sure
>>>>> > PreCommit passes when merging. A broken PreCommit means that we
>>>>> can't
>>>>> > merge any other changes with confidence.
>>>>> >
>>>>> > Thanks,
>>>>> > Max
>>>>> >
>>>>>
>>>>> --
>>>>> Jean-Baptiste Onofré
>>>>> jbonofre@apache.org
>>>>> http://blog.nanthrax.net
>>>>> Talend - http://www.talend.com
>>>>>
>>>>
Re: Python PreCommit broken
Posted by Charles Chen <cc...@google.com>.
Relatedly, https://github.com/apache/beam/pull/6151 also recently broke the
build (
https://builds.apache.org/view/A-D/view/Beam/job/beam_PostCommit_Java_GradleBuild/1503/console)
because the Precommits were very out of date when merged.
On Fri, Sep 21, 2018 at 12:50 PM Valentyn Tymofieiev <va...@google.com>
wrote:
> The change https://github.com/apache/beam/pull/6424 was not deemed
> particularly risky, and it's purpose was adding more tests to precommit
> test suite.
> There was a green Precommit signal on Jenkins, and I believe Postcommit
> test suite (at the same time) wouldn't catch this.
>
> The reason the breakage was introduced is that
> https://github.com/apache/beam/commit/7689f12db5 was committed after the
> PR 6424 was reviewed, but before it was merged. A combination of both
> introduced the breakage.
>
> Had we re-run the tests closer to the merge, we should have caught this.
> Can we automatically re-run precommits tests at merge time, when some of
> the files a PR is touching have changed since last precommit run?
>
> I suggest we proceed with https://github.com/apache/beam/pull/6464 or
> revert https://github.com/apache/beam/pull/6424 in the mean time, while
> we are iterating on the fix.
>
> On Fri, Sep 21, 2018 at 11:41 AM Charles Chen <cc...@google.com> wrote:
>
>> Do we happen to know the root cause for why this wasn't caught during
>> review / precommit?
>>
>> In the future, can we run manually run postcommits for risky changes like
>> these? That is, trigger it by commenting "Run Python PostCommit"?
>>
>> On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pa...@google.com>
>> wrote:
>>
>>> Robbe has prepared a better fix on
>>> https://github.com/apache/beam/pull/6465
>>> Hopefully that'll be the last of this breakage : )
>>> -P.
>>>
>>> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <jb...@nanthrax.net>
>>> wrote:
>>>
>>>> By the way, it fails for me on my machine as well.
>>>>
>>>> Regards
>>>> JB
>>>>
>>>> On 21/09/2018 18:10, Pablo Estrada wrote:
>>>> > I investigated. This failure comes from the activation of
>>>> > apache_beam.pipeline_test in Python 3 unit tests.
>>>> >
>>>> > I have https://github.com/apache/beam/pull/6464 out to fix this.
>>>> >
>>>> > In any case, it's very exciting that we have some unit tests running
>>>> on
>>>> > Py3 : )
>>>> > Best
>>>> > -P.
>>>> >
>>>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
>>>> > <ma...@apache.org>> wrote:
>>>> >
>>>> > Hi,
>>>> >
>>>> > The Python PreCommit tests are currently broken. Is anybody
>>>> looking
>>>> > into
>>>> > this?
>>>> >
>>>> > Example:
>>>> >
>>>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
>>>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>>>> >
>>>> > I'm sure this was just an accident. No big deal but let's make
>>>> sure
>>>> > PreCommit passes when merging. A broken PreCommit means that we
>>>> can't
>>>> > merge any other changes with confidence.
>>>> >
>>>> > Thanks,
>>>> > Max
>>>> >
>>>>
>>>> --
>>>> Jean-Baptiste Onofré
>>>> jbonofre@apache.org
>>>> http://blog.nanthrax.net
>>>> Talend - http://www.talend.com
>>>>
>>>
Re: Python PreCommit broken
Posted by Valentyn Tymofieiev <va...@google.com>.
The change https://github.com/apache/beam/pull/6424 was not deemed
particularly risky, and it's purpose was adding more tests to precommit
test suite.
There was a green Precommit signal on Jenkins, and I believe Postcommit
test suite (at the same time) wouldn't catch this.
The reason the breakage was introduced is that
https://github.com/apache/beam/commit/7689f12db5 was committed after the PR
6424 was reviewed, but before it was merged. A combination of both
introduced the breakage.
Had we re-run the tests closer to the merge, we should have caught this.
Can we automatically re-run precommits tests at merge time, when some of
the files a PR is touching have changed since last precommit run?
I suggest we proceed with https://github.com/apache/beam/pull/6464 or
revert https://github.com/apache/beam/pull/6424 in the mean time, while we
are iterating on the fix.
On Fri, Sep 21, 2018 at 11:41 AM Charles Chen <cc...@google.com> wrote:
> Do we happen to know the root cause for why this wasn't caught during
> review / precommit?
>
> In the future, can we run manually run postcommits for risky changes like
> these? That is, trigger it by commenting "Run Python PostCommit"?
>
> On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pa...@google.com> wrote:
>
>> Robbe has prepared a better fix on
>> https://github.com/apache/beam/pull/6465
>> Hopefully that'll be the last of this breakage : )
>> -P.
>>
>> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <jb...@nanthrax.net>
>> wrote:
>>
>>> By the way, it fails for me on my machine as well.
>>>
>>> Regards
>>> JB
>>>
>>> On 21/09/2018 18:10, Pablo Estrada wrote:
>>> > I investigated. This failure comes from the activation of
>>> > apache_beam.pipeline_test in Python 3 unit tests.
>>> >
>>> > I have https://github.com/apache/beam/pull/6464 out to fix this.
>>> >
>>> > In any case, it's very exciting that we have some unit tests running on
>>> > Py3 : )
>>> > Best
>>> > -P.
>>> >
>>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
>>> > <ma...@apache.org>> wrote:
>>> >
>>> > Hi,
>>> >
>>> > The Python PreCommit tests are currently broken. Is anybody looking
>>> > into
>>> > this?
>>> >
>>> > Example:
>>> >
>>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
>>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>>> >
>>> > I'm sure this was just an accident. No big deal but let's make sure
>>> > PreCommit passes when merging. A broken PreCommit means that we
>>> can't
>>> > merge any other changes with confidence.
>>> >
>>> > Thanks,
>>> > Max
>>> >
>>>
>>> --
>>> Jean-Baptiste Onofré
>>> jbonofre@apache.org
>>> http://blog.nanthrax.net
>>> Talend - http://www.talend.com
>>>
>>
Re: Python PreCommit broken
Posted by Charles Chen <cc...@google.com>.
Do we happen to know the root cause for why this wasn't caught during
review / precommit?
In the future, can we run manually run postcommits for risky changes like
these? That is, trigger it by commenting "Run Python PostCommit"?
On Fri, Sep 21, 2018 at 10:10 AM Pablo Estrada <pa...@google.com> wrote:
> Robbe has prepared a better fix on
> https://github.com/apache/beam/pull/6465
> Hopefully that'll be the last of this breakage : )
> -P.
>
> On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <jb...@nanthrax.net>
> wrote:
>
>> By the way, it fails for me on my machine as well.
>>
>> Regards
>> JB
>>
>> On 21/09/2018 18:10, Pablo Estrada wrote:
>> > I investigated. This failure comes from the activation of
>> > apache_beam.pipeline_test in Python 3 unit tests.
>> >
>> > I have https://github.com/apache/beam/pull/6464 out to fix this.
>> >
>> > In any case, it's very exciting that we have some unit tests running on
>> > Py3 : )
>> > Best
>> > -P.
>> >
>> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
>> > <ma...@apache.org>> wrote:
>> >
>> > Hi,
>> >
>> > The Python PreCommit tests are currently broken. Is anybody looking
>> > into
>> > this?
>> >
>> > Example:
>> >
>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
>> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>> >
>> > I'm sure this was just an accident. No big deal but let's make sure
>> > PreCommit passes when merging. A broken PreCommit means that we
>> can't
>> > merge any other changes with confidence.
>> >
>> > Thanks,
>> > Max
>> >
>>
>> --
>> Jean-Baptiste Onofré
>> jbonofre@apache.org
>> http://blog.nanthrax.net
>> Talend - http://www.talend.com
>>
>
Re: Python PreCommit broken
Posted by Pablo Estrada <pa...@google.com>.
Robbe has prepared a better fix on https://github.com/apache/beam/pull/6465
Hopefully that'll be the last of this breakage : )
-P.
On Fri, Sep 21, 2018 at 9:13 AM Jean-Baptiste Onofré <jb...@nanthrax.net>
wrote:
> By the way, it fails for me on my machine as well.
>
> Regards
> JB
>
> On 21/09/2018 18:10, Pablo Estrada wrote:
> > I investigated. This failure comes from the activation of
> > apache_beam.pipeline_test in Python 3 unit tests.
> >
> > I have https://github.com/apache/beam/pull/6464 out to fix this.
> >
> > In any case, it's very exciting that we have some unit tests running on
> > Py3 : )
> > Best
> > -P.
> >
> > On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
> > <ma...@apache.org>> wrote:
> >
> > Hi,
> >
> > The Python PreCommit tests are currently broken. Is anybody looking
> > into
> > this?
> >
> > Example:
> >
> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
> > JIRA: https://issues.apache.org/jira/browse/BEAM-5458
> >
> > I'm sure this was just an accident. No big deal but let's make sure
> > PreCommit passes when merging. A broken PreCommit means that we can't
> > merge any other changes with confidence.
> >
> > Thanks,
> > Max
> >
>
> --
> Jean-Baptiste Onofré
> jbonofre@apache.org
> http://blog.nanthrax.net
> Talend - http://www.talend.com
>
Re: Python PreCommit broken
Posted by Jean-Baptiste Onofré <jb...@nanthrax.net>.
By the way, it fails for me on my machine as well.
Regards
JB
On 21/09/2018 18:10, Pablo Estrada wrote:
> I investigated. This failure comes from the activation of
> apache_beam.pipeline_test in Python 3 unit tests.
>
> I have https://github.com/apache/beam/pull/6464 out to fix this.
>
> In any case, it's very exciting that we have some unit tests running on
> Py3 : )
> Best
> -P.
>
> On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mxm@apache.org
> <ma...@apache.org>> wrote:
>
> Hi,
>
> The Python PreCommit tests are currently broken. Is anybody looking
> into
> this?
>
> Example:
> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
> JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>
> I'm sure this was just an accident. No big deal but let's make sure
> PreCommit passes when merging. A broken PreCommit means that we can't
> merge any other changes with confidence.
>
> Thanks,
> Max
>
--
Jean-Baptiste Onofré
jbonofre@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com
Re: Python PreCommit broken
Posted by Thomas Weise <th...@apache.org>.
Pablo, thanks for taking this up.
It is indeed exciting to see things moving forward on the Python side
(state and timer support as well).
Still, we depend on the green CI status. Just to illustrate how breakage
can negatively affect even folks not immediately working on Python,
consider the portability work: For many changes we need the runner build
(such as Flink) as well as the Python SDK to be in good shape. Either of
the two breaks and the big search begins :)
Thanks,
Thomas
On Fri, Sep 21, 2018 at 9:11 AM Pablo Estrada <pa...@google.com> wrote:
> I investigated. This failure comes from the activation of
> apache_beam.pipeline_test in Python 3 unit tests.
>
> I have https://github.com/apache/beam/pull/6464 out to fix this.
>
> In any case, it's very exciting that we have some unit tests running on
> Py3 : )
> Best
> -P.
>
> On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mx...@apache.org> wrote:
>
>> Hi,
>>
>> The Python PreCommit tests are currently broken. Is anybody looking into
>> this?
>>
>> Example:
>>
>> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
>> JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>>
>> I'm sure this was just an accident. No big deal but let's make sure
>> PreCommit passes when merging. A broken PreCommit means that we can't
>> merge any other changes with confidence.
>>
>> Thanks,
>> Max
>>
>
Re: Python PreCommit broken
Posted by Pablo Estrada <pa...@google.com>.
I investigated. This failure comes from the activation of
apache_beam.pipeline_test in Python 3 unit tests.
I have https://github.com/apache/beam/pull/6464 out to fix this.
In any case, it's very exciting that we have some unit tests running on Py3
: )
Best
-P.
On Fri, Sep 21, 2018 at 6:40 AM Maximilian Michels <mx...@apache.org> wrote:
> Hi,
>
> The Python PreCommit tests are currently broken. Is anybody looking into
> this?
>
> Example:
>
> https://builds.apache.org/job/beam_PreCommit_Python_Commit/1308/#showFailuresLink
> JIRA: https://issues.apache.org/jira/browse/BEAM-5458
>
> I'm sure this was just an accident. No big deal but let's make sure
> PreCommit passes when merging. A broken PreCommit means that we can't
> merge any other changes with confidence.
>
> Thanks,
> Max
>