Skip to content

BigQuery: update signature for ML model access to label/feature columns. #5732

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.

Sign up for GitHub

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

Jump to bottom
Merged
shollyman merged 1 commit into from
Jul 18, 2019
Merged

BigQuery: update signature for ML model access to label/feature columns. #5732

shollyman merged 1 commit into from
Jul 18, 2019

Conversation

@shollyman
Copy link
Contributor

@shollyman shollyman commented Jul 12, 2019

This is a breaking change to a @BetaApi within BigQuery.

It swaps in a new veneer StandardSQLField type in place of the underlying
discovery type for getting information about the feature and label columns
from ML models, which was functionality introduced in:
#5214

@shollyman
BigQuery: update signature for ML model access to label/feature columns.
4d5b911 
This is a breaking change to a @BetaApi within BigQuery.

It swaps in a new veneer StandardSQLField type in place of the underlying
discovery type for getting information about the feature and label columns
from ML models, which was functionality introduced in:
googleapis#5214
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 12, 2019
@shollyman shollyman requested a review from tswast July 12, 2019 18:27
@codecov
Copy link

codecov bot commented Jul 12, 2019 •
edited
Loading

Codecov Report

Merging #5732 into master will decrease coverage by 0.38%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5732      +/-   ##
============================================
- Coverage     47.09%   46.71%   -0.39%     
+ Complexity    25063    23629    -1434     
============================================
  Files          2389     2389              
  Lines        259743   254614    -5129     
  Branches      29404    29257     -147     
============================================
- Hits         122320   118934    -3386     
+ Misses       128462   127626     -836     
+ Partials       8961     8054     -907
Impacted Files Coverage Δ Complexity Δ
...src/main/java/com/google/cloud/bigquery/Model.java 67.27% <ø> (ø) 12 <0> (ø) ⬇️
...main/java/com/google/cloud/bigquery/ModelInfo.java 75.53% <81.81%> (-0.58%) 21 <0> (ø)
...m/google/cloud/vision/v1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...e/cloud/vision/v1p4beta1/ImageAnnotatorClient.java 58.69% <0%> (-3.31%) 15% <0%> (-4%)
...e/cloud/vision/v1p3beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
...e/cloud/vision/v1p2beta1/ImageAnnotatorClient.java 47.05% <0%> (-2.95%) 9% <0%> (-2%)
.../java/com/google/cloud/speech/v1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...om/google/cloud/speech/v1p1beta1/SpeechClient.java 48.57% <0%> (-2.78%) 10% <0%> (-2%)
...d/webrisk/v1beta1/WebRiskServiceV1Beta1Client.java 63.41% <0%> (-2.5%) 12% <0%> (-3%)
...ogle/cloud/texttospeech/v1/TextToSpeechClient.java 55.88% <0%> (-2.46%) 9% <0%> (-2%)
... and 624 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d66c22b...4d5b911. Read the comment docs.

tswast
tswast approved these changes Jul 17, 2019
View reviewed changes
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. No tests to update? I guess I didn't really check for all properties on the Models API stats, either.

@shollyman
Copy link
Contributor Author

shollyman commented Jul 17, 2019

LGTM. No tests to update? I guess I didn't really check for all properties on the Models API stats, either.

The tests accessed the data transitively without declaring the intermediate types. The types changed but the getter names didn't. Ex:

 assertEquals(model.getFeatureColumns().get(0).getName(), "f1");
 assertEquals(model.getLabelColumns().get(0).getName(), "predicted_label");

kolea2
kolea2 previously requested changes Jul 18, 2019
View reviewed changes
google-cloud-clients/google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/Model.java

@Override
Builder setLabelColumns(List<StandardSqlField> labelColumnList) {
Builder setLabelColumns(List<StandardSQLField> labelColumnList) {
Copy link
Contributor

@kolea2 kolea2 Jul 18, 2019

Choose a reason for hiding this comment

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

Where is this annotated with @BetaApi? Just want to make sure we don't introduce a breaking change to something that wasn't labeled as such.

Copy link
Contributor Author

@shollyman shollyman Jul 18, 2019

Choose a reason for hiding this comment

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

The feature represents readonly properties provided by the BQ backend, so I marked it as BetaApi on the public getters in ModelInfo. The internal builder was not annotated this way.

Copy link
Contributor

@kolea2 kolea2 Jul 18, 2019

Choose a reason for hiding this comment

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

Ah, sorry didn't realize this wasn't a public method. SGTM.

@kolea2 kolea2 dismissed their stale review July 18, 2019 17:43

comment addressed

@shollyman shollyman merged commit 187fd3d into googleapis:master Jul 18, 2019
@shollyman shollyman deleted the modelschange branch July 18, 2019 18:24
@chingor13 chingor13 mentioned this pull request Jul 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tswast tswast tswast approved these changes

+1 more reviewer

@kolea2 kolea2 kolea2 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shollyman @tswast @kolea2 @googlebot