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

BigQuery: Set IPython user agent when running queries with IPython cell magic #8713

Merged
merged 4 commits into from
Jul 22, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Jul 19, 2019

Closes #8696.

How to test

See issue description - the Client should be instantiated with a custom user agent string when used from an IPython cell.

Things to discuss

  • A connection from the context could override this custom user agent string - is that expected behavior, or should the cell magic actually override the user agent string in context connections, too?
    No, this is primarily used with Kaggle datasets, thus the overridden user agent string should take precedence.

@plamut plamut added the api: bigquery Issues related to the BigQuery API. label Jul 19, 2019
@plamut plamut requested review from tswast and a team July 19, 2019 15:40
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 19, 2019
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

LGTM.

Test failures might be from an update to a dependency (pyarrow) that causes more warnings to be raised than expected.

We should see what those new warnings are and update the test to not care quite so much about them if they're safe to ignore.

A connection from the context could override this custom user agent string - is that expected behavior, or should the cell magic actually override the user agent string in context connections, too?

We added that _connection override feature for Kaggle public datasets, since they proxy connections to BigQuery. We have other ways to determine which requests come from Kaggle, and if anyone is proxying like Kaggle is doing, arguably we should keep whatever user-agent they have set, as it will be more specific than ours.

bigquery/google/cloud/bigquery/magics.py Outdated Show resolved Hide resolved
plamut added 2 commits July 19, 2019 20:07
Any additional warnings on top of the expected pyarrow warning would
cause one of the tests to fail. This commit makes that test more robust
by only focusing on the existence of the specific warning of interest.
@plamut
Copy link
Contributor Author

plamut commented Jul 19, 2019

The three new deprecation warnings that caused one of the test to fail were related to using some deprecated Pandas thingies, namely the RangeIndex attributes _start, _stop, and _step - these do not concern us?

I modified the test to only focus on the expected warning related to pyarrow.

@plamut
Copy link
Contributor Author

plamut commented Jul 19, 2019

Heh, now the coverage check is complaining. Will check and make the necessary adjustments.

@plamut plamut added needs work This is a pull request that needs a little love. and removed needs work This is a pull request that needs a little love. labels Jul 19, 2019
@plamut
Copy link
Contributor Author

plamut commented Jul 19, 2019

The coverage check complained, because not all paths have been taken in the next(...) construct in the test, go figure.

However, I did notice that even those three new warnings are caused by pyarrow. That was my bad, I initially only looked at w.message, but str(w) contains more info, e.g. the filename of the warning source, and that source is the pyarrow lib itself:

"{message : DeprecationWarning('RangeIndex._start is deprecated and will be removed in a future version. Use RangeIndex.start instead',), category :                     
'DeprecationWarning', filename : '/home/.../bigquery/.nox/unit-3-6/lib/python3.6/site-packages/pyarrow/pandas_compat.py', lineno : 383, line : None}"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquery Issues related to the BigQuery API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BigQuery: Add user-agent info to Jupyter magics Client constructor(s).
3 participants