feat: Add support for BigQuery Models. #2039

danielgsims · 2019-06-12T02:40:38Z

Updated discovery doc for bigquery
Created model class
Added model and models methods to BigQuery client
Added network calls to ConnectionInterface and Rest connection

danielgsims · 2019-06-12T03:10:06Z

@dwsupplee @jdpedrie - Here's the first draft of ML Models from BigQuery; opening as a draft PR like we discussed today.

I think one change I'd like to make is moving the fetch model functionality to dataset instead of being part of the BigQueryClient class since semantically it's a child of the model.

Are there any other suggestions, feedback or advice you two have before wrapping this up for a PR in the next couple of days?

danielgsims · 2019-06-12T03:10:52Z

See #1968

codecov · 2019-06-12T03:12:25Z

Codecov Report

Merging #2039 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #2039      +/-   ##
============================================
+ Coverage     92.57%   92.62%   +0.04%     
- Complexity     4379     4402      +23     
============================================
  Files           304      305       +1     
  Lines         13014    13078      +64     
============================================
+ Hits          12048    12113      +65     
+ Misses          966      965       -1

Impacted Files	Coverage Δ	Complexity Δ
BigQuery/src/Connection/Rest.php	`100% <100%> (ø)`	`26 <4> (+4)`	⬆️
BigQuery/src/Model.php	`100% <100%> (ø)`	`12 <12> (?)`
BigQuery/src/Dataset.php	`98.94% <100%> (+0.21%)`	`21 <3> (+3)`	⬆️
PubSub/src/Subscription.php	`100% <0%> (ø)`	`35% <0%> (+5%)`	⬆️
PubSub/src/Connection/Grpc.php	`97.56% <0%> (+0.48%)`	`37% <0%> (-1%)`	⬇️

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 27be9ef...9167426. Read the comment docs.

jdpedrie

looks good so far!

BigQuery/src/Connection/Rest.php

BigQuery/src/Model.php

jdpedrie · 2019-06-13T20:31:38Z

BigQuery/src/Model.php

+    public function update(array $metadata, array $options = [])
+    {
+        $options = $this->applyEtagHeader(
+            $options


nit: $options should be the last array in a list of merged arrays. That'll prevent the user from overriding something you set specifically in the client code.

Makes sense to me but Dataset has it written this way!

Making the change, maybe we should think about making the change in Dataset in the future too.

Indeed, that's just old code. I try to fix it when I touch the methods where it's in a less than optimal order.

dwsupplee

Looking nice, @danielgsims. Thanks for this :). Mostly housekeeping related comments.

BigQuery/src/Dataset.php

BigQuery/src/Model.php

BigQuery/tests/System/ManageModelsTest.php

BigQuery/tests/Unit/DatasetTest.php

BigQuery/tests/Unit/ModelTest.php

jdpedrie

looking good. mostly nits and small changes here.

BigQuery/src/Dataset.php

jdpedrie · 2019-06-25T20:54:59Z

BigQuery/src/Dataset.php

+        return new ItemIterator(
+            new PageIterator(
+                function (array $model) {
+                    return $this->model($model['modelReference']['modelId']);


Sorry I didn't raise this earlier, but the API call returns the model resource, it seems reasonable to inject that into the instance. That way, calls to Model::info() wouldn't require a second service call. If we don't hydrate the instance when we have the resource handy, there's no sense having $info as a Model constructor argument. This would require adding an $info arg to the Dataset::model() method, or you could do something like what we have in pubsub with a private factory.

Hey John, I ran into this one while developing. The data that is returned form the List call is not the full set of data that is returned when fetching the individual model resource. It seemed better to not include it so that you would consistently have the same data. In that case, should I just ditch it from the constructor and allow it to hydrate lazily?

I removed adding info from the constructor for now since it should always be triggered via the reload method. I'll keep an eye on this if the BQ team modifies the Models return response to include the full set of data.

Makes sense. Yeah, I think we can probably remove the argument if it’s unused. It’s nothing to add it back later if the situation changes.

The models.list documentation says the following:

Only the following fields are populated: modelReference, modelType, creationTime, lastModifiedTime and labels.

Which leads me to believe this will be a permanent decision to only return a subset of the information on list calls. Since the information returned appears useful to users, I'd like to propose we populate the Model object with the information when we have it, with documentation to let users know only a subset of the data is returned and to call reload on the Model if they would like access to all of the information.

BigQuery/src/Model.php

jdpedrie · 2019-06-25T21:04:26Z

BigQuery/src/Model.php

+    public function exists(array $options = [])
+    {
+        try {
+            $this->connection->getModel($this->identity + $options);


I know there are examples of direct connection calls in exists(), but I prefer calling Model::reload($options) here, and think we should make an effort to replace those eventually. Segregate connection calls as much as possible, and hydrate the instance in case this call is coming ahead of info().

I like that better. I can change the other parts of BigQuery in a future PR.

BigQuery/tests/Unit/DatasetTest.php

* Updated discovery doc for bigquery * Created model class * Added model and models methods to BigQuery\Dataset client * Added network calls to ConnectionInterface and Rest connection

dwsupplee · 2019-07-01T19:04:19Z

@danielgsims can you please ping me when this is ready for review?

BigQuery/tests/Snippet/ModelTest.php

dwsupplee · 2019-07-15T17:48:21Z

BigQuery/tests/System/ManageModelsTest.php

+        // Fetch the model and wait for it to be trained
+        $model = self::$dataset->model($modelId);
+
+        $backoff = new ExponentialBackoff(8);


Is this dead code then? Can we remove it please?

BigQuery/tests/System/ManageModelsTest.php

BigQuery/tests/Unit/ModelTest.php

googlebot · 2019-07-16T12:23:11Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

jdpedrie · 2019-07-16T12:23:35Z

@dwsupplee addressed your latest comments; PTAL!

googlebot · 2019-07-19T19:50:30Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

dwsupplee · 2019-07-19T19:52:26Z

BigQuery/src/Model.php

@@ -54,20 +54,26 @@ public function __construct(
        ConnectionInterface $connection,
        $id,
        $datasetId,
-        $projectId
+        $projectId,
+        $info = []


Please typehint to an array and add documentation.

dwsupplee · 2019-07-19T19:55:16Z

BigQuery/src/Dataset.php

        );
    }

    /**
     * Fetches all of the models in the dataset.
     *
+     * Please note that Model instances created by list calls may not contain a


Let's please be explicit about what fields are returned.

passed off to dws.

googlebot · 2019-07-19T20:08:04Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

dwsupplee · 2019-07-19T20:10:59Z

BigQuery/src/Model.php

+     * @param string $id The model's ID.
+     * @param string $datasetId The dataset's ID.
+     * @param string $projectId The project's ID.
+     * @param array $info


One more minor nit, could we please add a description?

googlebot · 2019-07-19T20:14:24Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

danielgsims added api: bigquery Issues related to the BigQuery API. needs work This is a pull request that needs a little love. labels Jun 12, 2019

danielgsims requested review from jdpedrie and dwsupplee June 12, 2019 02:40

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 12, 2019

jdpedrie reviewed Jun 13, 2019

View reviewed changes

danielgsims force-pushed the bigquery-models branch from b3cd981 to b8c8b8f Compare June 16, 2019 14:25

danielgsims removed the needs work This is a pull request that needs a little love. label Jun 18, 2019

dwsupplee suggested changes Jun 18, 2019

View reviewed changes

yoshi-automation added the 🚨 This issue needs some love. label Jun 19, 2019

dwsupplee added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Jun 20, 2019

danielgsims marked this pull request as ready for review June 25, 2019 13:11

danielgsims removed the needs work This is a pull request that needs a little love. label Jun 25, 2019

yoshi-automation added the 🚨 This issue needs some love. label Jun 25, 2019

danielgsims requested review from dwsupplee and jdpedrie June 25, 2019 13:36

dwsupplee added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Jun 25, 2019

jdpedrie previously requested changes Jun 25, 2019

View reviewed changes

feat: adds support for BigQuery ML Models

a64d07e

* Updated discovery doc for bigquery * Created model class * Added model and models methods to BigQuery\Dataset client * Added network calls to ConnectionInterface and Rest connection

danielgsims force-pushed the bigquery-models branch from f3d2b8e to a64d07e Compare June 28, 2019 05:03

jdpedrie changed the title ~~BigQuery Models~~ feat: Add support for BigQuery Models. Jun 28, 2019

danielgsims removed the needs work This is a pull request that needs a little love. label Jun 29, 2019

yoshi-automation added the 🚨 This issue needs some love. label Jun 29, 2019

fix: modifies order of params to prevent accidental overwrites

5fac570

yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019

dwsupplee suggested changes Jul 15, 2019

View reviewed changes

Address review feedback

df35642

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 16, 2019

jdpedrie added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 16, 2019

dwsupplee added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 19, 2019

dwsupplee suggested changes Jul 19, 2019

View reviewed changes

Address review comments

c92dcf5

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 19, 2019

dwsupplee suggested changes Jul 19, 2019

View reviewed changes

add param description

9167426

dwsupplee approved these changes Jul 19, 2019

View reviewed changes

dwsupplee added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 19, 2019

jdpedrie merged commit e2e006f into master Jul 22, 2019

jdpedrie deleted the bigquery-models branch July 22, 2019 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for BigQuery Models. #2039

feat: Add support for BigQuery Models. #2039

danielgsims commented Jun 12, 2019

danielgsims commented Jun 12, 2019

danielgsims commented Jun 12, 2019

codecov bot commented Jun 12, 2019 •

edited

Loading

jdpedrie left a comment

jdpedrie Jun 13, 2019

danielgsims Jun 16, 2019

danielgsims Jun 16, 2019

jdpedrie Jun 16, 2019

dwsupplee left a comment

jdpedrie left a comment

jdpedrie Jun 25, 2019

danielgsims Jun 26, 2019

danielgsims Jun 26, 2019

jdpedrie Jun 26, 2019

dwsupplee Jul 15, 2019

jdpedrie Jun 25, 2019

danielgsims Jun 26, 2019

dwsupplee commented Jul 1, 2019

dwsupplee Jul 15, 2019

googlebot commented Jul 16, 2019

jdpedrie commented Jul 16, 2019

googlebot commented Jul 19, 2019

dwsupplee Jul 19, 2019

dwsupplee Jul 19, 2019

googlebot commented Jul 19, 2019

dwsupplee Jul 19, 2019

googlebot commented Jul 19, 2019

feat: Add support for BigQuery Models. #2039

feat: Add support for BigQuery Models. #2039

Conversation

danielgsims commented Jun 12, 2019

danielgsims commented Jun 12, 2019

danielgsims commented Jun 12, 2019

codecov bot commented Jun 12, 2019 • edited Loading

Codecov Report

jdpedrie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwsupplee left a comment

Choose a reason for hiding this comment

jdpedrie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwsupplee commented Jul 1, 2019

Choose a reason for hiding this comment

googlebot commented Jul 16, 2019

jdpedrie commented Jul 16, 2019

googlebot commented Jul 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jul 19, 2019

Choose a reason for hiding this comment

googlebot commented Jul 19, 2019

codecov bot commented Jun 12, 2019 •

edited

Loading