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

Adding gcloud.bigtable package. #1122

Merged
merged 1 commit into from
Sep 10, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Sep 1, 2015

Initial commit just adds the basic import time check for gRPC and testing for all possible install states. It also adds a shadowed _testing/grpc that we can use to mock the installed library for tests.


We can discuss alternatives to mocking out grpc imports in dhermes/gcloud-python-bigtable#10 or right here. As far as I know, there is no way to globally (i.e. between / before modules) step in front of __import__ without doing some serious engineering.


@nathanielmanistaatgoogle Any better ideas than this one for allowing unit tests that "depend" on grpc to run? (None of the gRPC code is ever called in unit tests, just in system tests. Though the gRPC objects get mocked out, the imports need to work so nose doesn't get angry.)

@dhermes dhermes added the api: bigtable Issues related to the Bigtable API. label Sep 1, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 1, 2015
@nathanielmanistaatgoogle

I'm having a hard time finding my way around this - what problem are we solving?

@dhermes
Copy link
Contributor Author

dhermes commented Sep 2, 2015

Point of this PR: Add the basic gcloud.bigtable subpackage. (No functionality yet, but will be bringing it over piecemeal from https://github.com/dhermes/gcloud-python-bigtable / http://gcloud-python-bigtable.readthedocs.org/en/latest/)


Extra helpful in this PR: In gcloud.bigtable package __init__.py, add a helpful error message if grpc is installed but the gRPC core system libs are not on the LD_LIBRARY_PATH.


Problem to solve (thing I asked Nathaniel about): Importing gRPC in unit tests (in a virtualenv created by tox) without actually installing https://pypi.python.org/pypi/grpcio or grpc core.


Dan's solution to above problem: Make a fake grpc package and add it to the PYTHONPATH in the test environment (thanks to setenv, a tox feature)

@dhermes
Copy link
Contributor Author

dhermes commented Sep 2, 2015

@tseaver Any thoughts?

@nathanielmanistaatgoogle Does my last comment clarify the purpose?

@nathanielmanistaatgoogle

It helps! I'm still not completely clear though - why bend over backwards to function in an environment in which "the gRPC core system libs are not on the LD_LIBRARY_PATH"? If you're testing that gcloud-python is still usable-at-least-in-part in such a configuration, why not catch just ImportErrors where they come up and verify the limited functionality to be verified?

What's the relationship between gcloud-python and grpcio? Full dependency, or weird kind-of dependency, or something else?

I'm not saying anything's wrong here; I just don't yet understand.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 3, 2015

We don't want users of gcloud.datastore to have to deal with the requirements of gcloud.bigtable. Thus grpcio will not be required and we want a way to gracefully instruct users of gcloud.bigtable to install grpcio.

This is really just a stop-gap until users can reliably install gRPC core and grpcio.

@nathanielmanistaatgoogle

Okay. Then it's crazy, but once one has bought into its particular craziness, there's nothing wrong with it. Since it's a stop-gap I counsel filing a clean-up bug and including at least one TODO(issue XXX): remove this temporary artifice in the code.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 3, 2015

RE: "crazy". I am very open to other ideas, if you have any.

This just seemed like the only way to rely on an "upstream" dependency without hurting users that didn't need / want it.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 8, 2015

@tseaver WDYT here?

@nathanielmanistaatgoogle

The only other idea I have is "catch a raised ImportError", with the thought that that is what I think I've heard is "usually" done in these kinds of situations. Why is that the wrong thing in this case?

@@ -11,6 +11,8 @@ deps =
nose
unittest2
protobuf==3.0.0-alpha-1
setenv =
PYTHONPATH = {toxinidir}/_testing

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 8, 2015

@nathanielmanistaatgoogle "catch a raised ImportError" is exactly what we do, though doing it in unit tests would require digging into the guts of nosetests and likely making our own custom nose test runner() (i.e. writing code instead of scripting). This is not a path I wish to go down.

@nathanielmanistaatgoogle

Thanks for saying that; I think I get it now.

Initial commit just adds the basic import time check for
gRPC and testing for all possible install states. It
also adds a shadowed _testing/grpc that we can use to
mock the installed library for tests.
@dhermes dhermes force-pushed the bigtable-grpc-import branch from 26be1c8 to 75809ae Compare September 10, 2015 20:37
@dhermes
Copy link
Contributor Author

dhermes commented Sep 10, 2015

@tseaver PTAL. I dropped the test___init__.py module altogether, since the package is just an import and a pragma no cover. I also using print(..., file=sys.stderr) instead of raising a different exception.

The _testing/ directory and the use of it in tox.ini are the only other remaining changes.

@tseaver
Copy link
Contributor

tseaver commented Sep 10, 2015

LGTM

dhermes added a commit that referenced this pull request Sep 10, 2015
@dhermes dhermes merged commit 61abd0c into googleapis:master Sep 10, 2015
@dhermes dhermes deleted the bigtable-grpc-import branch September 10, 2015 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants