Skip to content
This repository was archived by the owner on Nov 12, 2025. It is now read-only.

chore: transition the library to microgenerator - #62

Merged
tswast merged 21 commits into
googleapis:masterfrom
plamut:iss-61
Sep 24, 2020
Merged

chore: transition the library to microgenerator#62
tswast merged 21 commits into
googleapis:masterfrom
plamut:iss-61

Conversation

@plamut

@plamut plamut commented Sep 18, 2020

Copy link
Copy Markdown
Contributor

Closes #61.

Remaining things:

  • Add the UPGRADING guide to docs.
  • Fix failing tests
  • Decide what to do with v1beta1 and v1beta2 code (remove? YES!).
  • Samples tests not running on Kokoro?
@google-cla google-cla Bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 18, 2020
@product-auto-label product-auto-label Bot added the api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API. label Sep 19, 2020
@plamut
plamut force-pushed the iss-61 branch 2 times, most recently from c252fba to 38e6c39 Compare September 22, 2020 13:12
@plamut
plamut marked this pull request as ready for review September 22, 2020 13:49
@plamut
plamut requested a review from a team September 22, 2020 13:49
@plamut
plamut requested a review from a team as a code owner September 22, 2020 13:49
@plamut
plamut requested review from busunkim96, dinagraves and tswast and removed request for a team September 22, 2020 13:49
Comment thread .kokoro/release/common.cfg
Comment thread google/cloud/bigquery/storage/__init__.py Outdated
Comment thread google/cloud/bigquery/storage/__init__.py Outdated
# limitations under the License.
#

from .services.big_query_read import BigQueryReadClient

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's have this be the manual wrapper.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a generated file and bigquery_storage_v1/client.py imports from it. The latter assumes that the generated client is imported and wraps it, although it could also import directly from storage_v1.services.big_query_read, in principle.

Do you think it's worth maintaining additional synth replacement rules to prevent users from accessing the generated client through google.cloud.bigquery.storage_v1?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think it's worth maintaining additional synth replacement rules to prevent users from accessing the generated client through google.cloud.bigquery.storage_v1?

I'm pretty sure that's what I did for the bigquery_storage_v1beta1 package (that is, exclude the equivalent top-level __init__.py from getting copied over.

I'm still flip-flopping about what namespace we should use for the manual clients. It's nice to avoid breaking changes, but if we're breaking anyway, maybe we should be consistent. What do other manual libs do for namespace of manual clients?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PubSub just exposes the generated clients through pubsub_v1.__init__.py import.

I don't know about other libs, but can check (I suspect they left these imports intact, though).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting! I find that it uses the google.pubsub_v1 for generated and google.cloud.pubsub_v1 for handwritten to be pretty confusing, but I guess so long as it's documented well, it'll be okay.

@plamut plamut Sep 23, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can at least mention this in the UPGRADING guide and just recommend to import from google.cloud.bigquery.storage (the samples use this path, too). This will give us more freedom to restructure the library internally in the future (in theory at least).

Update: FWIW, I found another library with hand-written layer that has already been migrated to microgenerator (AutoML) and it, too, exposes the generated client through google.cloud.automl_v1.

Comment thread google/cloud/bigquery_storage_v1/types.py
Comment thread samples/quickstart/quickstart.py Outdated
Comment thread synth.py
@plamut
plamut requested a review from tswast September 23, 2020 13:47
Comment thread samples/to_dataframe/main_test.py
Comment thread docs/index.rst Outdated
Comment thread google/cloud/bigquery_storage_v1/reader.py Outdated
Comment thread noxfile.py Outdated
Comment thread google/cloud/bigquery_storage_v1/async_client.py Outdated
@plamut

plamut commented Sep 23, 2020

Copy link
Copy Markdown
Contributor Author

Addressed all comments. Once the changes are finalized and confirmed, I will follow up with the UPGRADING guide.

Update: Added the upgrade guide, as I think nothing significant will change anymore now that we discussed the import paths.

@tswast

tswast commented Sep 24, 2020

Copy link
Copy Markdown
Contributor

Regarding the Sample tests, I just sent out internal CL 333505519 to add the missing Kokoro config.

Comment thread UPGRADING.md
Comment thread UPGRADING.md Outdated

@tswast tswast left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM once those UPGRADING fixes are in.

@tswast
tswast merged commit 0a0eb2e into googleapis:master Sep 24, 2020
@plamut
plamut deleted the iss-61 branch September 24, 2020 15:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

api: bigquerystorage Issues related to the googleapis/python-bigquery-storage API. cla: yes This human has signed the Contributor License Agreement.

2 participants