Skip to content
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

Fix basic errors in integration/run_tests.py #70

Merged
merged 1 commit into from
Jan 17, 2018

Conversation

arostamianfar
Copy link
Contributor

Note: The tests still do not run successfully e2e (table deletion fails when cleaning up), but they progress further :)

Issue #62

setup.py Outdated
@@ -21,6 +21,8 @@
# This is needed due to https://issues.apache.org/jira/browse/BEAM-3357.
'grpcio>=1.3.5,<=1.7.3',
'apache-beam[gcp]>=2.2',
'google-api-python-client>=1.6',
Copy link
Member

Choose a reason for hiding this comment

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

nit: Do you mind adding a note with a reference to Issue #71 here?

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.

setup.py Outdated
@@ -21,6 +21,8 @@
# This is needed due to https://issues.apache.org/jira/browse/BEAM-3357.
'grpcio>=1.3.5,<=1.7.3',
'apache-beam[gcp]>=2.2',
'google-api-python-client>=1.6',
'google-cloud-bigquery>=0.25',
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that version 0.25 works or should this be > 0.25 (see my TODO above)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch. Done. It installed v0.29.0 on my machine.

self.dataset = None

def __enter__(self):
client = bigquery.Client(project=self.project)
dataset_ref = client.dataset(self.dataset_id)
# TODO(bashir2): There is a mismatch between the bigquery API versions
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove this TODO now. BTW, I thought we need a version strictly greater than 0.25.

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. And good catch! I changed to be >0.25.

bashir2
bashir2 previously approved these changes Jan 16, 2018
Copy link
Member

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

Just a minor comment and thanks for fixing this.

setup.py Outdated
@@ -21,6 +21,11 @@
# This is needed due to https://issues.apache.org/jira/browse/BEAM-3357.
'grpcio>=1.3.5,<=1.7.3',
'apache-beam[gcp]>=2.2',
# Note that adding 'google-api-python-client>=1.6' causes some dependency
# mismatch issues if using 'setup.py install'. See Issue #71.
Copy link
Member

Choose a reason for hiding this comment

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

nit: dependency issues are there no matter we use setup.py install or not, the difference seems to be that setup.py install stops while pip install continues but still installs conflicting versions.

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.

bashir2
bashir2 previously approved these changes Jan 16, 2018
Copy link
Member

@bashir2 bashir2 left a comment

Choose a reason for hiding this comment

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

LGTM again to make GitHub happy :-)

bashir2
bashir2 previously approved these changes Jan 17, 2018
@arostamianfar arostamianfar merged commit ed60457 into googlegenomics:master Jan 17, 2018
@arostamianfar arostamianfar deleted the inttest branch February 13, 2018 20:04
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.

2 participants