Re: Python sdist broken?

[Mark Liu]

Thanks for reporting the problem and share your thoughts Michael! Kasia
suggested a fix  to my dev
branch. My roll-forward is out in https://github.com/apache/beam/pull/8067,
which contains a fix that excludes any possible build/target/dist in
sub-directory when copy SDK source code. (see line 1619

)

As for testing, I should have been more careful since it's a fundamental
change that essentially affects all Python builds/tests. And I agree with
Michael about Gradle task vs subproject. In my own experience in Python
SDK, adding a test suite generally just need to create a new Gradle task
since most of the testing steps are either provided or done by executing
command line.

Mark

On Thu, Mar 14, 2019 at 5:59 PM Valentyn Tymofieiev 
wrote:

> Thanks, Kasia for noticing this and suggesting a solution. I couldn't find
> the suggestion on Mark's PR or the rollback PR, but sounds like we have a
> plan.
> I agree with Michael that this seems like a problem that would be hard to
> catch in precommit since we don't build all the projects in a precommit,
> and one of the projects had a dependency on sdist task, which was changed.
>
> On Thu, Mar 14, 2019 at 4:05 PM Michael Luckey 
> wrote:
>
>> Thanks Katarzyna for that explanation.
>>
>> On Thu, Mar 14, 2019 at 8:37 PM Katarzyna Kucharczyk <
>> ka.kucharc...@gmail.com> wrote:
>>
>>> Hi,
>>> Michael - many thanks for letting know and Ahmet for reverting. I think
>>> I know which task caused the problem.
>>>
>>> A quick explanation about what's happened. Load test gradle task was
>>> created in load_tests module. A build/ directory which was excluded in
>>> 'sdist' task wasn't the same as the one created inside the module. While
>>> copying it this created some kind of loop. I already suggested my solution
>>> to Mark.
>>>
>>> @Valentyn, the load_tests (in smoke option) haven't been added to
>>> preCommit as not important to check in a different way than on demand (by
>>> phrase triggering). Anyway, this situation shows that such decision should
>>> be re-thought. Also, a solution here would be moving load_test task to the
>>> main gradle python file. I decided to keep them separately because they
>>> weren't designed to be part of big task such as preCommit.
>>>
>>
>> We should keep in mind, that creating a submodule is in fact really a
>> heavyweight process. Whereas adding a task should be considered pretty
>> lightweight.IIUC, that load test module only adds a single task, which
>> might be called with different args. If so, that task is already 'kept
>> separate' in itself.
>>
>> So if this kind of separation is the only reason - in contrast to a more
>> semantic 'this *is* a different subproject' we probably should restrain
>> from creating as such. But my understanding could be wrong here.
>>
>> Regarding the miss out with pre commit test, it is unfortunately not
>> enough to rely on some subset of tasks if doing such heavy lifting on the
>> build system. Even a full build will not reveal all problems, as gradle
>> only configures and of course runs what is asked for. There is probably not
>> much we can do about that apart from executing everything, which seems
>> unfeasible.
>>
>>
>>>
>>> Let me know what you think.
>>>
>>> Kasia
>>>
>>> On Thu, Mar 14, 2019 at 7:32 PM Michael Luckey 
>>> wrote:
>>>
 Thanks for that revert!

 I d really love to get those proposed changes of #7675 in, as this
 '--keep-temp' also causes some troubles during build.

 Please let me know if/how I could support here.

 michel

 On Thu, Mar 14, 2019 at 5:21 PM Ahmet Altay  wrote:

> Sent https://github.com/apache/beam/pull/8059 to revert #7675.
>
> On Thu, Mar 14, 2019 at 8:10 AM Valentyn Tymofieiev <
> valen...@google.com> wrote:
>
>> This most likely caused by https://github.com/apache/beam/pull/7675.
>> I suggest we revert and roll-forward with a fix. We should also 
>> understand
>> why this didn't surface in the precommit tests.
>>
>> On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey 
>> wrote:
>>
>>> Hi,
>>>
>>> while trying to get build working on my vanilla vm, I encounter
>>> strangely failing python sdists tasks. It keeps complaining about being
>>> unable to copy files. Those files are in a deeply recursive structure 
>>> which
>>> led me to the assumption this might not be intended. This seems to be
>>> caused by merge of [1], not verified though.
>>>
>>> This also happens on our nightly snapshot build [2]. Anyone else
>>> having this problem?
>>>
>>> thx,
>>>
>>> michel
>>>
>>> [1] https://github.com/apache/beam/pull/7675
>>> [2] https://scans.gradle.com/s/rllnbwj4lj4qc
>>>
>>>
>>>
>>

Re: Python sdist broken?

[Valentyn Tymofieiev]

Thanks, Kasia for noticing this and suggesting a solution. I couldn't find
the suggestion on Mark's PR or the rollback PR, but sounds like we have a
plan.
I agree with Michael that this seems like a problem that would be hard to
catch in precommit since we don't build all the projects in a precommit,
and one of the projects had a dependency on sdist task, which was changed.

On Thu, Mar 14, 2019 at 4:05 PM Michael Luckey  wrote:

> Thanks Katarzyna for that explanation.
>
> On Thu, Mar 14, 2019 at 8:37 PM Katarzyna Kucharczyk <
> ka.kucharc...@gmail.com> wrote:
>
>> Hi,
>> Michael - many thanks for letting know and Ahmet for reverting. I think I
>> know which task caused the problem.
>>
>> A quick explanation about what's happened. Load test gradle task was
>> created in load_tests module. A build/ directory which was excluded in
>> 'sdist' task wasn't the same as the one created inside the module. While
>> copying it this created some kind of loop. I already suggested my solution
>> to Mark.
>>
>> @Valentyn, the load_tests (in smoke option) haven't been added to
>> preCommit as not important to check in a different way than on demand (by
>> phrase triggering). Anyway, this situation shows that such decision should
>> be re-thought. Also, a solution here would be moving load_test task to the
>> main gradle python file. I decided to keep them separately because they
>> weren't designed to be part of big task such as preCommit.
>>
>
> We should keep in mind, that creating a submodule is in fact really a
> heavyweight process. Whereas adding a task should be considered pretty
> lightweight.IIUC, that load test module only adds a single task, which
> might be called with different args. If so, that task is already 'kept
> separate' in itself.
>
> So if this kind of separation is the only reason - in contrast to a more
> semantic 'this *is* a different subproject' we probably should restrain
> from creating as such. But my understanding could be wrong here.
>
> Regarding the miss out with pre commit test, it is unfortunately not
> enough to rely on some subset of tasks if doing such heavy lifting on the
> build system. Even a full build will not reveal all problems, as gradle
> only configures and of course runs what is asked for. There is probably not
> much we can do about that apart from executing everything, which seems
> unfeasible.
>
>
>>
>> Let me know what you think.
>>
>> Kasia
>>
>> On Thu, Mar 14, 2019 at 7:32 PM Michael Luckey 
>> wrote:
>>
>>> Thanks for that revert!
>>>
>>> I d really love to get those proposed changes of #7675 in, as this
>>> '--keep-temp' also causes some troubles during build.
>>>
>>> Please let me know if/how I could support here.
>>>
>>> michel
>>>
>>> On Thu, Mar 14, 2019 at 5:21 PM Ahmet Altay  wrote:
>>>
 Sent https://github.com/apache/beam/pull/8059 to revert #7675.

 On Thu, Mar 14, 2019 at 8:10 AM Valentyn Tymofieiev <
 valen...@google.com> wrote:

> This most likely caused by https://github.com/apache/beam/pull/7675.
> I suggest we revert and roll-forward with a fix. We should also understand
> why this didn't surface in the precommit tests.
>
> On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey 
> wrote:
>
>> Hi,
>>
>> while trying to get build working on my vanilla vm, I encounter
>> strangely failing python sdists tasks. It keeps complaining about being
>> unable to copy files. Those files are in a deeply recursive structure 
>> which
>> led me to the assumption this might not be intended. This seems to be
>> caused by merge of [1], not verified though.
>>
>> This also happens on our nightly snapshot build [2]. Anyone else
>> having this problem?
>>
>> thx,
>>
>> michel
>>
>> [1] https://github.com/apache/beam/pull/7675
>> [2] https://scans.gradle.com/s/rllnbwj4lj4qc
>>
>>
>>
>

Re: Python sdist broken?

[Michael Luckey]

Thanks Katarzyna for that explanation.

On Thu, Mar 14, 2019 at 8:37 PM Katarzyna Kucharczyk <
ka.kucharc...@gmail.com> wrote:

> Hi,
> Michael - many thanks for letting know and Ahmet for reverting. I think I
> know which task caused the problem.
>
> A quick explanation about what's happened. Load test gradle task was
> created in load_tests module. A build/ directory which was excluded in
> 'sdist' task wasn't the same as the one created inside the module. While
> copying it this created some kind of loop. I already suggested my solution
> to Mark.
>
> @Valentyn, the load_tests (in smoke option) haven't been added to
> preCommit as not important to check in a different way than on demand (by
> phrase triggering). Anyway, this situation shows that such decision should
> be re-thought. Also, a solution here would be moving load_test task to the
> main gradle python file. I decided to keep them separately because they
> weren't designed to be part of big task such as preCommit.
>

We should keep in mind, that creating a submodule is in fact really a
heavyweight process. Whereas adding a task should be considered pretty
lightweight.IIUC, that load test module only adds a single task, which
might be called with different args. If so, that task is already 'kept
separate' in itself.

So if this kind of separation is the only reason - in contrast to a more
semantic 'this *is* a different subproject' we probably should restrain
from creating as such. But my understanding could be wrong here.

Regarding the miss out with pre commit test, it is unfortunately not enough
to rely on some subset of tasks if doing such heavy lifting on the build
system. Even a full build will not reveal all problems, as gradle only
configures and of course runs what is asked for. There is probably not much
we can do about that apart from executing everything, which seems
unfeasible.


>
> Let me know what you think.
>
> Kasia
>
> On Thu, Mar 14, 2019 at 7:32 PM Michael Luckey 
> wrote:
>
>> Thanks for that revert!
>>
>> I d really love to get those proposed changes of #7675 in, as this
>> '--keep-temp' also causes some troubles during build.
>>
>> Please let me know if/how I could support here.
>>
>> michel
>>
>> On Thu, Mar 14, 2019 at 5:21 PM Ahmet Altay  wrote:
>>
>>> Sent https://github.com/apache/beam/pull/8059 to revert #7675.
>>>
>>> On Thu, Mar 14, 2019 at 8:10 AM Valentyn Tymofieiev 
>>> wrote:
>>>
 This most likely caused by https://github.com/apache/beam/pull/7675. I
 suggest we revert and roll-forward with a fix. We should also understand
 why this didn't surface in the precommit tests.

 On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey 
 wrote:

> Hi,
>
> while trying to get build working on my vanilla vm, I encounter
> strangely failing python sdists tasks. It keeps complaining about being
> unable to copy files. Those files are in a deeply recursive structure 
> which
> led me to the assumption this might not be intended. This seems to be
> caused by merge of [1], not verified though.
>
> This also happens on our nightly snapshot build [2]. Anyone else
> having this problem?
>
> thx,
>
> michel
>
> [1] https://github.com/apache/beam/pull/7675
> [2] https://scans.gradle.com/s/rllnbwj4lj4qc
>
>
>

Re: Python sdist broken?

[Katarzyna Kucharczyk]

Hi,
Michael - many thanks for letting know and Ahmet for reverting. I think I
know which task caused the problem.

A quick explanation about what's happened. Load test gradle task was
created in load_tests module. A build/ directory which was excluded in
'sdist' task wasn't the same as the one created inside the module. While
copying it this created some kind of loop. I already suggested my solution
to Mark.

@Valentyn, the load_tests (in smoke option) haven't been added to preCommit
as not important to check in a different way than on demand (by phrase
triggering). Anyway, this situation shows that such decision should be
re-thought. Also, a solution here would be moving load_test task to the
main gradle python file. I decided to keep them separately because they
weren't designed to be part of big task such as preCommit.

Let me know what you think.

Kasia

On Thu, Mar 14, 2019 at 7:32 PM Michael Luckey  wrote:

> Thanks for that revert!
>
> I d really love to get those proposed changes of #7675 in, as this
> '--keep-temp' also causes some troubles during build.
>
> Please let me know if/how I could support here.
>
> michel
>
> On Thu, Mar 14, 2019 at 5:21 PM Ahmet Altay  wrote:
>
>> Sent https://github.com/apache/beam/pull/8059 to revert #7675.
>>
>> On Thu, Mar 14, 2019 at 8:10 AM Valentyn Tymofieiev 
>> wrote:
>>
>>> This most likely caused by https://github.com/apache/beam/pull/7675. I
>>> suggest we revert and roll-forward with a fix. We should also understand
>>> why this didn't surface in the precommit tests.
>>>
>>> On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey 
>>> wrote:
>>>
 Hi,

 while trying to get build working on my vanilla vm, I encounter
 strangely failing python sdists tasks. It keeps complaining about being
 unable to copy files. Those files are in a deeply recursive structure which
 led me to the assumption this might not be intended. This seems to be
 caused by merge of [1], not verified though.

 This also happens on our nightly snapshot build [2]. Anyone else having
 this problem?

 thx,

 michel

 [1] https://github.com/apache/beam/pull/7675
 [2] https://scans.gradle.com/s/rllnbwj4lj4qc



>>>

Re: Python sdist broken?

[Michael Luckey]

Thanks for that revert!

I d really love to get those proposed changes of #7675 in, as this
'--keep-temp' also causes some troubles during build.

Please let me know if/how I could support here.

michel

On Thu, Mar 14, 2019 at 5:21 PM Ahmet Altay  wrote:

> Sent https://github.com/apache/beam/pull/8059 to revert #7675.
>
> On Thu, Mar 14, 2019 at 8:10 AM Valentyn Tymofieiev 
> wrote:
>
>> This most likely caused by https://github.com/apache/beam/pull/7675. I
>> suggest we revert and roll-forward with a fix. We should also understand
>> why this didn't surface in the precommit tests.
>>
>> On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey 
>> wrote:
>>
>>> Hi,
>>>
>>> while trying to get build working on my vanilla vm, I encounter
>>> strangely failing python sdists tasks. It keeps complaining about being
>>> unable to copy files. Those files are in a deeply recursive structure which
>>> led me to the assumption this might not be intended. This seems to be
>>> caused by merge of [1], not verified though.
>>>
>>> This also happens on our nightly snapshot build [2]. Anyone else having
>>> this problem?
>>>
>>> thx,
>>>
>>> michel
>>>
>>> [1] https://github.com/apache/beam/pull/7675
>>> [2] https://scans.gradle.com/s/rllnbwj4lj4qc
>>>
>>>
>>>
>>

Re: Python sdist broken?

[Ahmet Altay]

Sent https://github.com/apache/beam/pull/8059 to revert #7675.

On Thu, Mar 14, 2019 at 8:10 AM Valentyn Tymofieiev 
wrote:

> This most likely caused by https://github.com/apache/beam/pull/7675. I
> suggest we revert and roll-forward with a fix. We should also understand
> why this didn't surface in the precommit tests.
>
> On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey 
> wrote:
>
>> Hi,
>>
>> while trying to get build working on my vanilla vm, I encounter strangely
>> failing python sdists tasks. It keeps complaining about being unable to
>> copy files. Those files are in a deeply recursive structure which led me to
>> the assumption this might not be intended. This seems to be caused by merge
>> of [1], not verified though.
>>
>> This also happens on our nightly snapshot build [2]. Anyone else having
>> this problem?
>>
>> thx,
>>
>> michel
>>
>> [1] https://github.com/apache/beam/pull/7675
>> [2] https://scans.gradle.com/s/rllnbwj4lj4qc
>>
>>
>>
>

Re: Python sdist broken?

[Valentyn Tymofieiev]

This most likely caused by https://github.com/apache/beam/pull/7675. I
suggest we revert and roll-forward with a fix. We should also understand
why this didn't surface in the precommit tests.

On Thu, Mar 14, 2019 at 4:44 AM Michael Luckey  wrote:

> Hi,
>
> while trying to get build working on my vanilla vm, I encounter strangely
> failing python sdists tasks. It keeps complaining about being unable to
> copy files. Those files are in a deeply recursive structure which led me to
> the assumption this might not be intended. This seems to be caused by merge
> of [1], not verified though.
>
> This also happens on our nightly snapshot build [2]. Anyone else having
> this problem?
>
> thx,
>
> michel
>
> [1] https://github.com/apache/beam/pull/7675
> [2] https://scans.gradle.com/s/rllnbwj4lj4qc
>
>
>

Python sdist broken?

[Michael Luckey]

Hi,

while trying to get build working on my vanilla vm, I encounter strangely
failing python sdists tasks. It keeps complaining about being unable to
copy files. Those files are in a deeply recursive structure which led me to
the assumption this might not be intended. This seems to be caused by merge
of [1], not verified though.

This also happens on our nightly snapshot build [2]. Anyone else having
this problem?

thx,

michel

[1] https://github.com/apache/beam/pull/7675
[2] https://scans.gradle.com/s/rllnbwj4lj4qc