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

Add python 3.4 #580

Merged
merged 2 commits into from
Feb 5, 2015
Merged

Add python 3.4 #580

merged 2 commits into from
Feb 5, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jan 30, 2015

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

Notes:

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 30, 2015
@tseaver 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
@tseaver
Copy link
Contributor Author

tseaver commented Jan 30, 2015

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

@dhermes
Copy link
Contributor

dhermes commented Jan 30, 2015

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

@dhermes
Copy link
Contributor

dhermes commented Jan 30, 2015

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.

@@ -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.

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@tseaver
Copy link
Contributor Author

tseaver commented Feb 3, 2015

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
Copy link
Contributor

@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
Copy link
Contributor Author

tseaver commented Feb 3, 2015

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

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

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

@tseaver
Copy link
Contributor Author

tseaver commented Feb 3, 2015

@craigcitro
Copy link
Contributor

thanks @tseaver !

@tseaver
Copy link
Contributor Author

tseaver commented Feb 3, 2015

@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
Copy link

Coverage Status

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

@dhermes
Copy link
Contributor

dhermes commented Feb 4, 2015

@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.

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.

This comment was marked as spam.

@coveralls
Copy link

Coverage Status

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

@dhermes
Copy link
Contributor

dhermes commented Feb 4, 2015

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
Copy link

Coverage Status

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

@tseaver
Copy link
Contributor Author

tseaver commented Feb 4, 2015

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
Copy link

Coverage Status

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

@dhermes
Copy link
Contributor

dhermes commented Feb 4, 2015

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
Copy link
Contributor

dhermes commented Feb 4, 2015

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

tseaver added a commit that referenced this pull request Feb 5, 2015
@tseaver tseaver merged commit 9518db7 into googleapis:master Feb 5, 2015
@tseaver tseaver deleted the add-python-3.4 branch February 5, 2015 00:19
@tseaver
Copy link
Contributor Author

tseaver commented Feb 5, 2015

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
Copy link
Contributor Author

tseaver commented Feb 5, 2015

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

@brettcannon
Copy link

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
Copy link
Contributor Author

tseaver commented Feb 5, 2015

@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
Copy link
Contributor

dhermes commented Feb 20, 2015

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
Copy link
Contributor

dhermes commented Feb 20, 2015

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.

vchudnov-g pushed a commit that referenced this pull request Sep 20, 2023
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants