Skip to content

Conversation

@markflyhigh
Copy link
Contributor

@markflyhigh markflyhigh commented Mar 15, 2019

This PR is to roll-forward #7675 with fixes suggested by @kkucharc:

  • Avoid recursive copy of non-source files under sdks/python/apache_beam in BeamModulePlugin.groovy line 1619.
  • sdist task failed to collect singleFile due to existence of srcs under build directory. Fix at BeamModulePlugin.groovy line 1642. Error looks like:
Caused by: java.lang.IllegalStateException: Expected directory '/Users/kasia/Repos/beam/sdks/python/build' to contain exactly one file, however, it contains more than one file.
        at org.gradle.api.internal.file.AbstractFileCollection.getSingleFile(AbstractFileCollection.java:65)
        at org.apache.beam.gradle.BeamModulePlugin$_apply_closure20$_closure136$_closure145.doCall(BeamModulePlugin.groovy:1644)

+R: @aaltay


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Python Build Status
Build Status
--- Build Status
Build Status
Build Status --- --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@markflyhigh
Copy link
Contributor Author

Run Gradle Publish

@markflyhigh
Copy link
Contributor Author

This PR is ready for review.

./gradlew publish(nightly snapshot job) is failed due to reach to Jenkins job timeout (140 mins). No error is fund in Python tests.

@markflyhigh
Copy link
Contributor Author

+cc: @kkucharc @ajamato @tvalentyn

@markflyhigh
Copy link
Contributor Author

Run Gradle Publish

1 similar comment
@tvalentyn
Copy link
Contributor

Run Gradle Publish

@tvalentyn
Copy link
Contributor

LGTM, not sure why gradle publish failed. It would be easier to review if you separated new changes and previously reviewed commit into separate commit, but looks like filepaths are the only things that changed.

@tvalentyn
Copy link
Contributor

Run Gradle Publish

@adude3141
Copy link
Contributor

not sure why gradle publish failed.

As @markflyhigh already stated, the publish task is killed by Jenkins after 140 min (2h20min). Not sure what did cause the build time increase here. Probably has nothing to do with this commit, but was hidden before as build was already failing on earlier tasks.

So we probably need to increase that timeout, at least temporarily, until we are able to improve test performance.

@aaltay
Copy link
Member

aaltay commented Mar 19, 2019

Is the test error a flake? Can we rerun it?

@adude3141
Copy link
Contributor

I do not believe its a flake. The whole build just takes more than 140mins on Jenkins right now.

This might be caused by some added tests. The last successful [1] publish on Feb, 24 took 132mins already, so it seems we need to increase the timeout here.

@markflyhigh Could you increase this timeout with this PR, as - at least to my understanding - the build fails on master till we get this PR merged

I have no idea how much is required, but I d go for 3 (or even 4?) hours.

[1] https://builds.apache.org/job/beam_Release_NightlySnapshot/344/

@markflyhigh
Copy link
Contributor Author

Yes, it doesn't seem to be a flake. I'll increase the timeout to 200mins in this PR to see if the job can pass successfully. I feel 3 - 4 hours is acceptable currently since it only runs once a day.

@markflyhigh
Copy link
Contributor Author

Run Gradle Publish

@markflyhigh
Copy link
Contributor Author

Run Seed Job

@markflyhigh
Copy link
Contributor Author

Run Gradle Publish

@markflyhigh
Copy link
Contributor Author

Run Python PreCommit

@markflyhigh
Copy link
Contributor Author

Run Seed Job

@markflyhigh
Copy link
Contributor Author

Run Gradle Publish

@markflyhigh
Copy link
Contributor Author

Run Java PreCommit

@markflyhigh markflyhigh requested a review from aaltay March 21, 2019 00:30
@markflyhigh
Copy link
Contributor Author

markflyhigh commented Mar 21, 2019

"./gradlew publish" failed since python36 and 37 are missing on Beam12. This is an infra issue and I started a discussion on dev@. Gradle scan shows that python load test (failed previously and led to rollback) was built successfully.

PTAL @aaltay @tvalentyn @adude3141

@adude3141
Copy link
Contributor

Run Gradle Publish

1 similar comment
@adude3141
Copy link
Contributor

Run Gradle Publish

@markflyhigh
Copy link
Contributor Author

markflyhigh commented Mar 21, 2019

This time nightly snapshot failed (job link) on publishMavenJavaPublicationToMavenRepository in many java module. According to the build history of this job, it's been failed/broken since Feb.

IMHO, this PR shouldn't be blocked on nightly snapshot job if it consistently failed on irrelevant reasons and consider existing build time of Python Precommit (>1 hour).

@markflyhigh
Copy link
Contributor Author

Run Python Load Tests Smoke

@tvalentyn
Copy link
Contributor

LGTM

@markflyhigh
Copy link
Contributor Author

Run Python PostCommit

@markflyhigh
Copy link
Contributor Author

I think current tests are good enough for the coverage:

  • precommit
  • postcommit2/3
  • load tests
  • Portable_Python
  • Python_PVR_Flink

Can we merge it? @aaltay @adude3141

@adude3141
Copy link
Contributor

Thanks, @markflyhigh, I have to admit, you put lot of effort into testing this. And it is most likely, that no other will pass the publish.

But as it improves the overall performance, lets merge and fix outstanding issues separately.

Again, thanks a lot for the effort you put into this.

@adude3141 adude3141 merged commit 58a70b2 into apache:master Mar 22, 2019
@adude3141
Copy link
Contributor

@markflyhigh
While running build on my local machine, I've seen tons of additional log output cluttering console.

Is there any option to quieten that? It is difficult to follow and I prefer to add that '--info' resp '--debug' flags if required.

@markflyhigh
Copy link
Contributor Author

Do you want to quite/control logs that are generated from python commands? According to Gradle doc: Gradle redirects anything written to standard output to its logging system at the QUIET log level. Seems there is no option to disable this level.

I think you may need to do it inside python and then integrate with Gradle if possible.

@adude3141
Copy link
Contributor

Comparing e.g. java precommit build [1] with python [2] I was under the impression, that the log output heavily increased with this commit.

Just wondering, if you got the same feeling. From my point of view, most output is not helpful, e.g. all this 'creating...', 'copying', 'adding' just clutters console.

You are right regarding gradles default behaviour. But we might consider adapting that to something more sensible on task level? Either by quietening the tools delegated to - which might have some drawbacks - or we could add a
logging.captureStandardOutput org.gradle.api.logging.LogLevel.INFO
(or even better DEBUG?) to the 'sdist' task which will quieten things a bit. And similar stuff with other tasks.

Did not look deeper into that, though. And of course, it has a lot to do with daily dev - which I am not doing on python currently -, whether that output should be seen or could be considered clutter (which might be enabled if required by adding '-I' or '-q' param.

[1] https://builds.apache.org/job/beam_PreCommit_Java_Phrase/827/consoleFull
[2] https://builds.apache.org/job/beam_PreCommit_Python_Commit/5194/consoleFull

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.

5 participants