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

Remove gRPC/GAX conditional checks. #2308

Merged
merged 1 commit into from
Sep 15, 2016
Merged

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Sep 12, 2016

Adding support for an environment variable that App Engine users can use to disable installing gRPC packages into their application.

Follow-up to #2304.

@jonparrott Can you weigh in here on the GAE env. var. in setup.py?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 12, 2016
@dhermes
Copy link
Contributor Author

dhermes commented Sep 12, 2016

@theacodes
Copy link
Contributor

What's prompting this change? This is essentially switching to grpc as a hard requirement for this library.

@omaray should probably chime in, too.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

@jonparrott I touched an old part in #2304 that sparked the discussion. gRPC is already a hard requirement in Python 2.7.

Very soon it won't be in google-cloud, it'll be in google-cloud-bigtable, google-cloud-pubsub, etc. so it's not that worrisome.

I just wanted your input on the env. var. to shut it down.

@theacodes
Copy link
Contributor

Any dynamic behavior in setup.py will force you to stop providing wheels
(for py2.7 in this case, at least).

Otherwise whatever. App engine standard really just needs to add grpc
support and we probably need to write a custom script to handle installing
this package properly.

On Mon, Sep 12, 2016, 6:33 PM Danny Hermes notifications@github.com wrote:

@jonparrott https://github.com/jonparrott I touched an old part in #2304
#2304
that sparked the discussion. gRPC is already a hard requirement in Python
2.7.

Very soon it won't be in google-cloud, it'll be in google-cloud-bigtable,
google-cloud-pubsub, etc. so it's not that worrisome.

I just wanted your input on the env. var. to shut it down.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2308 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPUcy8F66KFoXSbUqCRz2k-L_KfWWjiks5qpf17gaJpZM4J7G6R
.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

force you to stop providing wheels

How so?

@theacodes
Copy link
Contributor

Wheels don't run setup.py at all.

On Mon, Sep 12, 2016, 6:40 PM Danny Hermes notifications@github.com wrote:

force you to stop providing wheels

How so?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2308 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPUc76mbcgxFFGX-B3jZZqpJGrAs5Geks5qpf7_gaJpZM4J7G6R
.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

@jonparrott So your concern is that setup.py wouldn't be invoked by someone trying to install into a GAE environment?


Worth noting: I canceled the travis-ci/push check since it came from creating a branch. Though in reality I only created the branch for the AppVeyor check.

@theacodes
Copy link
Contributor

@dhermes exactly

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

But for the purposes of you writing docs, couldn't you include --no-use-wheel or --no-binary?

This discussion may become moot when we make each service it's own package? But for services like pubsub and logging, gRPC should be the default unless it can't be, so we'll probably add gRPC as a dependency in google-cloud-pubsub, etc.

Any suggestions for how to play nice with everyone (GAE, using wheels, etc.)?

@theacodes
Copy link
Contributor

Just be sure to publish an sdist as well as a wheel for now. I really have no idea what our strategy is going to be for standard.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

@jonparrott SGTM, I'll tear out the env. var. and we can punt for now.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

I deleted the branch and AppVeyor didn't trigger on my new push w/o the GAE env. var.

https://ci.appveyor.com/project/GoogleCloudPlatform/google-cloud-python/build/1.0.664.followup-2304 still a good indicator that the changes work on Windows

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

@tseaver PTAL

@@ -28,8 +28,9 @@
'grpc-google-logging-v2 >= 0.8.0, < 0.9dev',
]

if sys.version_info[:2] == (2, 7) and 'READTHEDOCS' not in os.environ:
REQUIREMENTS.extend(GRPC_EXTRAS)
RTD_ENV_VAR = 'READTHEDOCS'

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@tseaver
Copy link
Contributor

tseaver commented Sep 13, 2016

I'm confused: aren't we losing our can't-install-gRPC fallback here?

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

@tseaver What is our "our can't-install-gRPC fallback"?

@tseaver
Copy link
Contributor

tseaver commented Sep 13, 2016

@dhermes Until this PR, installing gcloud-python without also installing gcloud-python[grpc] still allows use of all of the APIs except Bigtable. On platforms where those extras cannot be installed for some reason, the library is still useful: even Dubsub, Datastore, and Logging fall back to using HTTP.

With this change, users who cannot install grpcio and the GAX wrappers will no longer be able to install the library. GAE is the obvious case, but maybe users on older LTS releases will have the same issues.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 13, 2016

@tseaver

  1. That's already not true on 2.7 (i.e. if they are on 2.7 we always add the gRPC deps, it is this way because Python 3 install was b0rked at the time)
  2. Once we move to google-cloud-* packages, users can opt in to the packages they want, some of which will have gRPC and some which won't. Though for packages with a fallback, we still have to solve this problem.

@dhermes
Copy link
Contributor Author

dhermes commented Sep 15, 2016

@tseaver Bump.

@tseaver
Copy link
Contributor

tseaver commented Sep 15, 2016

LGTM. I'll let @jonparrott deal with the pitchfork-wielding GAE mob, then. :)

@dhermes dhermes merged commit 6f09ebd into googleapis:master Sep 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. grpc packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants