Skip to content

[BEAM-7578] add py37 hdfs integration test#8970

Merged
udim merged 2 commits intoapache:masterfrom
Juta:hdfs
Jul 16, 2019
Merged

[BEAM-7578] add py37 hdfs integration test#8970
udim merged 2 commits intoapache:masterfrom
Juta:hdfs

Conversation

@Juta
Copy link
Contributor

@Juta Juta commented Jul 1, 2019

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- Build Status --- --- Build Status
Java Build Status 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 --- --- Build Status

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status Build Status Build Status
Portable --- Build Status --- ---

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

@Juta
Copy link
Contributor Author

Juta commented Jul 1, 2019

Run Python PostCommit

@Juta
Copy link
Contributor Author

Juta commented Jul 1, 2019

Run Python PostCommit

@Juta
Copy link
Contributor Author

Juta commented Jul 1, 2019

@tvalentyn could you help me, I am trying to figure out why this test won't run? It says Task with path ':sdks:python:test-suites:hdfs:py35:hdfsIntegrationTest' not found in root project 'beam'. (in PostCommit on py3) Can you see what I am doing wrong?

@tvalentyn
Copy link
Contributor

I think you'd need to update https://github.com/apache/beam/blob/master/settings.gradle#L135 to include the new project to make this work in current structure, however, since this is a direct runner test, let's keep it within direct runner integration test suites.

# By default it runs wordcount against a locally accessible HDFS service.
# See hdfs_integration_test.sh for example usage.

FROM python:3.5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make base image a parameter here and pass it externally when we run docker to build the container image? This file could start like:

ARG BASE_IMAGE
FROM $BASE_IMAGE

however we'd need to figure out the gradle bits and the location of the dockerfile so that we could call it with multiple python versions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'd modify the python version in-place and double check that this test passes before doing major changes to the test/config structure. Perhaps you've already done that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping @tvalentyn , I made changes to the test structure to pass the python version as an argument, all hdfs tests are passing

@Juta
Copy link
Contributor Author

Juta commented Jul 2, 2019

Run Python PostCommit

@Juta Juta force-pushed the hdfs branch 3 times, most recently from e10b771 to 56b697a Compare July 2, 2019 08:53
@Juta
Copy link
Contributor Author

Juta commented Jul 2, 2019

Run Python PostCommit

@Juta
Copy link
Contributor Author

Juta commented Jul 2, 2019

Run Portable_Python PreCommit

@Juta
Copy link
Contributor Author

Juta commented Jul 2, 2019

R: @tvalentyn

@Juta
Copy link
Contributor Author

Juta commented Jul 2, 2019

Run Python PreCommit

Copy link
Contributor

@tvalentyn tvalentyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Juta, left a minor comment, also, let's change HDFS test to run with Python 3.7 instead of 3.5. If we pick one version I'd go with Python 3.7 at this point. Also as discussed offline, I think one Python 3 suite is sufficient for this test for now.

trap finally EXIT

time docker-compose ${COMPOSE_OPT} build
time docker-compose ${COMPOSE_OPT} build --build-arg BASE_IMAGE=$1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please call out arguments to this script in a comment on top of the file? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sorry if I wasn't clear - I actually thought of adding a comment earlier at the beginning of the script:

It could be a description of required arguments or sample usage for someone looking into how to run that script manually.

@Juta
Copy link
Contributor Author

Juta commented Jul 8, 2019

Run Python PostCommit

@Juta
Copy link
Contributor Author

Juta commented Jul 9, 2019

Run Python PostCommit

@Juta
Copy link
Contributor Author

Juta commented Jul 9, 2019

Run Python PreCommit

Copy link
Contributor

@tvalentyn tvalentyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Juta!
@udim, could you help squash+merge this please?

@tvalentyn
Copy link
Contributor

We can also rename the PR to s/py35/py37, a committer or an author could do it.

@Juta Juta changed the title [BEAM-7578] add py35 hdfs integration test [BEAM-7578] add py37 hdfs integration test Jul 15, 2019
@tvalentyn
Copy link
Contributor

@udim, could you help squash+merge this please?

@udim udim merged commit 88c3f6b into apache:master Jul 16, 2019
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.

3 participants