-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-5315] Partially port Avroio module #7644
Conversation
41be2c6
to
45bbefb
Compare
Tests currently fail due to |
retest this please |
1 similar comment
retest this please |
Thanks @RobbeSneyders . @aaltay or @charlesccychen - please help with the merge. Thank you! |
hey @RobbeSneyders could you resolve the conflict please? |
sdks/python/tox.ini
Outdated
commands = | ||
python --version | ||
pip --version | ||
{toxinidir}/scripts/run_tox_cleanup.sh | ||
python setup.py nosetests --tests {[testenv:py3]modules} | ||
python apache_beam/examples/complete/autocomplete_test.py | ||
python setup.py nosetests --ignore-files vcfio_test\.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we skip VCF IO tests in Python 3, to keep tox.ini cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean let's add @skip
statement to the test class in vcfio_test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good.
commands = | ||
python --version | ||
pip --version | ||
{toxinidir}/scripts/run_tox_cleanup.sh | ||
python setup.py nosetests --tests {[testenv:py3]modules} | ||
python apache_beam/examples/complete/autocomplete_test.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this line shouldn't be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is executed in all other tox environments. I don't know why it is added separately, so I also don't know why we shouldn't do it in Python 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per 245facd we try to execute one test outside of the test suite to make sure nothing fails if we do so.
PTAL |
LGTM. Thank you. |
Thanks, this LGTM. |
This is is part of a series of PRs with goal to make Apache Beam PY3 compatible. The proposal with the outlined approach has been documented here: https://s.apache.org/beam-python-3.
This PR partially ports the avroio module.
4 tests (2x the same 2 tests, for Avro and FastAvro) are still failing. I created BEAM-6522 to track this.
Post-Commit Tests Status (on master branch)