Add python 3.4 #580

tseaver · 2015-01-30T19:58:52Z

Default 'tox' all passes with this branch, including 'py34'.

Notes:

Pre-applies changes from https://github.com/craigcitro/apitools/pull/15 into vendored tree.
Adds (but doesn't enable by default) a 'tox -e regression3' target. Those tests are currently failing.

tseaver · 2015-01-30T19:59:53Z

Shouldn't merge until we fix the 'regression3' failures.

dhermes · 2015-01-30T20:25:27Z

How do the non-Python 3 supported deps work with this?

dhermes · 2015-01-30T20:27:52Z

I notice

I: 25, 0: Locally disabling import-error (F0401) (locally-disabled)

Didn't we bend over backwards to avoid locally disabled lint errors in our code? Tools should serve us, not the other way around?

The httplib import is currently failing. It should be six.moves.http_client now.

gcloud/datastore/key.py

@@ -102,7 +101,7 @@ def _parse_path(path_args):
            id_or_name_list += (partial_ending,)

        result = []
-        for kind, id_or_name in izip(kind_list, id_or_name_list):
+        for kind, id_or_name in zip(kind_list, id_or_name_list):


coveralls · 2015-02-03T19:21:14Z

Coverage remained the same at 100.0% when pulling 2bafde0 on tseaver:add-python-3.4 into 32b7853 on GoogleCloudPlatform:master.

coveralls · 2015-02-03T19:56:50Z

Coverage remained the same at 100.0% when pulling 7c1ba1b on tseaver:add-python-3.4 into e80958a on GoogleCloudPlatform:master.

tseaver · 2015-02-03T20:18:10Z

Unit tests are all passing (rebase is a great way to "lose" bugfixes, BTW).

tox -e regression3 failures are all due to a 400: invalid grant response, percolating out of oauth2client.client._do_refresh_request.

@craigcitro, do you have oauth2client actually running against the backend under Py3k (using the GOOGLE_APPLICATION_CREDENTIALS mechanism), as opposed to just its unit tests?

craigcitro · 2015-02-03T20:59:48Z

@tseaver sadly, no, no one's had the time to set up integration tests for oauth2client and python3.

there is an open PR that may be relevant: googleapis/oauth2client#104 ... if that would/does fix your issue, that would be interesting to know.

cc @anthmgoogle

tseaver · 2015-02-03T21:11:36Z

@craigcitro, neither googleapis/oauth2client#104 nor jcgregorio/httplib2#291 make the regression tests pass under Py3k. :(

craigcitro · 2015-02-03T21:12:23Z

@tseaver :( ... can you grab a stack trace and file an issue against oauth2client?

tseaver · 2015-02-03T21:20:36Z

@craigcitro: googleapis/oauth2client#125

craigcitro · 2015-02-03T21:22:09Z

thanks @tseaver !

tseaver · 2015-02-03T21:22:13Z

@dhermes remarked:

How do the non-Python 3 supported deps work with this?

I'm not sure what you mean? protobuf 3.0.0-alpha1 fixes our last Python2-only dependency. If we decide to adopt it (alpha, warts, and all), then we're good.

@dhermes PTAL: I'd like to merge soonish to avoid chasing rebase pain forever, even if regression3 doesn't yet pass.

coveralls · 2015-02-03T21:53:37Z

Coverage remained the same at 100.0% when pulling fa73670 on tseaver:add-python-3.4 into cecdc4f on GoogleCloudPlatform:master.

dhermes · 2015-02-04T17:32:48Z

@tseaver Reviewing now. I agree we should focus on this before anything else to avoid more rebase pain. Let's not merge anything else until this is in; don't let me forget.

I assume we can merge but can't endorse until the issues in https://github.com/google/oauth2client are resolved.

gcloud/datastore/test___init__.py

+        environ = {_DATASET_ENV_VAR_NAME: implicit_dataset_id}
+        return _Monkey(os, getenv=environ.get)
+
+    def _monkeyImplicit(self, connection=None, app_identity=None):


coveralls · 2015-02-04T18:57:30Z

Coverage remained the same at 100.0% when pulling e769de3 on tseaver:add-python-3.4 into cecdc4f on GoogleCloudPlatform:master.

dhermes · 2015-02-04T19:13:19Z

Overall the change looks fine (I made some comments):

33 commits is way too many to introduce into history for such a straightforward change. I'd expect 1 to 2 at most. (I realize rebasing and straddling fixes one-by-one is the reason for the changes.) Can you git merge --squash into a fresh checkout from master?
The protobuf people have renamed 3.0.0-alpha1 -> 3.0.0-alpha-1 (try tox --recreate to see)
Though pylint is annoying when we get cut on sharp corners, I am not a fan of abandoning our policy of disallowed pylint: disable statements. I spent a TON of time getting us to a point where we didn't need them. I made a few small changes to make this possible: dhermes@4cf536e
Why are we checking in gcloud/datastore/_datastore_v1.proto? I assume you have it because the new version of protobuf required regenerating gcloud/datastore/_datastore_v1_pb2.py.
I tracked down the reason for the oauth2client failures in the bug you filed and will take a stab.
I assume you left the protobuf pinned dependency out of setup.py since we can't endorse py34 yet? (We can't endorse until regression3 passes, which can't happen until invalid_grant failures under Python3 oauth2client#125 is fixed.)

- Pin protobuf == 3.0.0-alpha1 - Clarify text vs. bytes in upload / download as string, in response bodies, in 'bytes_field' proto values (e.g. 'transaction'). - Add a copy of the protobuf spec file and regenerate protobuf wrapper with 'protoc' 3.0.0-alpha1. - Transaction ID must be bytes. - Straddle 'range', 'native' strings. URL quoting / encoding / parsing. integer division, 'httplib'. - Pre-apply changes from /craigcitro/apitools#15. We will need to reload the files once Craig makes a new release (or even better, remove the vendoring). - Work around missing 'contextlib.nested' in Py3k. - Disable lint's failed attempt to import 'six.moves'. See: https://bitbucket.org/logilab/pylint/issue/200/ etc. - Add support for regression testing w/ Python 3.4. Note that these tests are currently *failing* due to googleapis/oauth2client#125

coveralls · 2015-02-04T19:29:57Z

Coverage remained the same at 100.0% when pulling 8ab8c1b on tseaver:add-python-3.4 into cecdc4f on GoogleCloudPlatform:master.

tseaver · 2015-02-04T19:54:08Z

Can you git merge --squash into a fresh checkout from master?

Now squashed.

The protobuf people have renamed 3.0.0-alpha1 -> 3.0.0-alpha-1 (try tox --recreate to see)

I tried, and didn't see failures. https://travis-ci.org/GoogleCloudPlatform/gcloud-python/builds/49500349 passes without it (I'm pretty sure it doesn't reuse environments). 0e99cea updates it just in case.

Though pylint is annoying when we get cut on sharp corners, I am not a fan of abandoning our policy of disallowed pylint: disable statements. I spent a TON of time getting us to a point where we didn't need them. I made a few small changes to make this possible: dhermes/google-cloud-python@4cf536e

This isn't a "sharp corner" due to a difference of opinion, it is a known bug in pylint. Please see this
gist, showing the changes I'm trying to make to accomodate your wishes, and the new linting errors they introduce. We are objectively making the code harder to read / understand in order to appease brokenness in the tool: this is a case where disabling the tool is the Right Thing(TM).

Why are we checking in gcloud/datastore/_datastore_v1.proto? I assume you have it because the new version of protobuf required regenerating gcloud/datastore/_datastore_v1_pb2.py.

Yes, and I envision regenerating it whenever we pick up new versions of protobuf (to ensure that we match the expectations of the library from the generated code). It is also much better "local" documentation than the generated module.

I tracked down the reason for the oauth2client failures in the bug you filed and will take a stab.

Cool.

I assume you left the protobuf pinned dependency out of setup.py since we can't endorse py34 yet? (We can't endorse until regression3 passes, which can't happen until googleapis/oauth2client#125 is fixed.)

I'd really like them to get 3.0.0 to a "final" release, or at least a feature-frozen / API stable beta: I'm uncomfortable with us depending so strongly on an alpha.

coveralls · 2015-02-04T19:57:13Z

Coverage remained the same at 100.0% when pulling 0e99cea on tseaver:add-python-3.4 into cecdc4f on GoogleCloudPlatform:master.

dhermes · 2015-02-04T22:45:33Z

RE: The "3.0.0-alpha1 -> 3.0.0-alpha-1" rename, when running locally I got HTTP 404s. Maybe Travis has a local mirror of PyPI?

RE: pylint I ran into the same issues in your gist and resolved them in dhermes@4cf536e.

I wasn't trying to say the sharp corner was about opinion, rather about the tool cutting us if we hold it the wrong way (due to issues with pylint). The problem is a mix of

being stuck straddling Py2 / Py3 (necessitating six)
six being tricky by creating modules (rather than importing)
the pylint feature of only statically analyzing files (instead of importing)
some WTF behavior of pylint with Argument 'safe' passed by position and keyword in function call (redundant-keyword-arg)

I don't think any of the "solutions" are pleasing (including using six), but am worried that the massive work I did to ensure we didn't use pylint: disable will be for naught. It's a slippery slope to just go back to using pylint: disable whenever lint complains to us. By actually addressing these issues during review / pre-submit, we more often than not end up with higher quality code.

At any rate, keeping out pylint: disable is very doable. Recall the original motivation; that our code doesn't serve our tools.

dhermes · 2015-02-04T22:52:33Z

@tseaver Let's punt on the pylint: disable discussion and merge this. LGTM.

dhermes · 2015-02-04T22:55:02Z

~~TYPO: Look~~ I'm looking into pylint --py3k (some work from Brett Cannon):

Add python 3.4

tseaver · 2015-02-05T00:20:32Z

FWIW, the quote I saw linked was,

tools should serve the developers and their code, not the other way 'round.

Which doesn't mean that we aren't in a "smelly" place (straddling sucks).

tseaver · 2015-02-05T00:23:24Z

I did see @brettcannon trying to get some traction on https://bitbucket.org/logilab/pylint/issue/200

brettcannon · 2015-02-05T13:49:03Z

So pylint --py3k gets around the issue that Tres directly linked to by running only transition-related checkers and turning the rest off. So using six shouldn't trigger any warnings if you use the ``--py3k` option for Pylint.

tseaver · 2015-02-05T14:42:29Z

@brettcannon Cool, only we are using it for automating more general style checks. Until the disabling comments on the six.moves imports added by this PR, we have been able to avoid adding such comments.

dhermes · 2015-02-20T22:24:07Z

Running https://gist.github.com/dhermes/d193bf8e9a14f24d8750 results in

Diff base not specified, listing all files in repository.
Status code:
6
----------------------------------------
Actual Errors (132):
----------------------------------------
import missing `from __future__ import absolute_import` (no-absolute-import) (123)
print statement used (print-statement) (9)

So this isn't quite what I had hoped but instead is as Brett said:

only transition-related checkers and turning the rest off

Worth knowing but doesn't fix our use of the pylint: disable messages

dhermes · 2015-02-20T22:46:06Z

This is known to break ignored-modules in pylintrc:
https://bitbucket.org/logilab/pylint/issue/223/ignored-modules-should-turn-no-name-in
(Throws up hands)

I followed the issue so hopefully at some point they fix it and we can ditch the pylint: disable. Le sigh.

…le.com/agent-assist/docs/summarization) (#580) * feat: Add Agent Assist Summarization API (https://cloud.google.com/agent-assist/docs/summarization) docs: clarify SuggestionFeature enums which are specific to chat agents PiperOrigin-RevId: 477479918 Source-Link: googleapis/googleapis@6deca98 Source-Link: googleapis/googleapis-gen@b23d242 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYjIzZDI0MmM1YzVkZWI3Y2U2ZjRhN2Y3MTg2MmEzMTE1NTUwNjliZCJ9 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 30, 2015

tseaver added api: datastore Issues related to the Datastore API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. api: storage Issues related to the Cloud Storage API. milestone-feature labels Jan 30, 2015

dhermes reviewed Jan 30, 2015
View reviewed changes

gcloud/datastore/key.py

@@ -102,7 +101,7 @@ def _parse_path(path_args):

id_or_name_list += (partial_ending,)

result = []

for kind, id_or_name in izip(kind_list, id_or_name_list):

for kind, id_or_name in zip(kind_list, id_or_name_list):

This comment was marked as spam.

Sign in to view

jgeewax removed the milestone-feature label Jan 30, 2015

tseaver removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 3, 2015

tseaver mentioned this pull request Feb 3, 2015

invalid_grant failures under Python3 googleapis/oauth2client#125

Closed

dhermes reviewed Feb 4, 2015
View reviewed changes

gcloud/datastore/test___init__.py

environ = {_DATASET_ENV_VAR_NAME: implicit_dataset_id}

return _Monkey(os, getenv=environ.get)

def _monkeyImplicit(self, connection=None, app_identity=None):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Accomodate the new canonical protobuf release spelling.

0e99cea

dhermes mentioned this pull request Feb 4, 2015

Add script to re-build _datastore_v1_pb2.py from .proto #594

Closed

tseaver added a commit that referenced this pull request Feb 5, 2015

Merge pull request #580 from tseaver/add-python-3.4

9518db7

Add python 3.4

tseaver merged commit 9518db7 into googleapis:master Feb 5, 2015

tseaver deleted the add-python-3.4 branch February 5, 2015 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add python 3.4 #580

Add python 3.4 #580

tseaver commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

dhermes commented Jan 30, 2015

This comment was marked as spam.

coveralls commented Feb 3, 2015

coveralls commented Feb 3, 2015

tseaver commented Feb 3, 2015

craigcitro commented Feb 3, 2015

tseaver commented Feb 3, 2015

craigcitro commented Feb 3, 2015

tseaver commented Feb 3, 2015

craigcitro commented Feb 3, 2015

tseaver commented Feb 3, 2015

coveralls commented Feb 3, 2015

dhermes commented Feb 4, 2015

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Feb 4, 2015

dhermes commented Feb 4, 2015

coveralls commented Feb 4, 2015

tseaver commented Feb 4, 2015

coveralls commented Feb 4, 2015

dhermes commented Feb 4, 2015

dhermes commented Feb 4, 2015

dhermes commented Feb 4, 2015

tseaver commented Feb 5, 2015

tseaver commented Feb 5, 2015

brettcannon commented Feb 5, 2015

tseaver commented Feb 5, 2015

dhermes commented Feb 20, 2015

dhermes commented Feb 20, 2015

Add python 3.4 #580

Add python 3.4 #580

Conversation

tseaver commented Jan 30, 2015

tseaver commented Jan 30, 2015

dhermes commented Jan 30, 2015

dhermes commented Jan 30, 2015

This comment was marked as spam.

coveralls commented Feb 3, 2015

coveralls commented Feb 3, 2015

tseaver commented Feb 3, 2015

craigcitro commented Feb 3, 2015

tseaver commented Feb 3, 2015

craigcitro commented Feb 3, 2015

tseaver commented Feb 3, 2015

craigcitro commented Feb 3, 2015

tseaver commented Feb 3, 2015

coveralls commented Feb 3, 2015

dhermes commented Feb 4, 2015

This comment was marked as spam.

This comment was marked as spam.

coveralls commented Feb 4, 2015

dhermes commented Feb 4, 2015

coveralls commented Feb 4, 2015

tseaver commented Feb 4, 2015

coveralls commented Feb 4, 2015

dhermes commented Feb 4, 2015

dhermes commented Feb 4, 2015

dhermes commented Feb 4, 2015

tseaver commented Feb 5, 2015

tseaver commented Feb 5, 2015

brettcannon commented Feb 5, 2015

tseaver commented Feb 5, 2015

dhermes commented Feb 20, 2015

dhermes commented Feb 20, 2015