You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/02/09 03:07:57 UTC

[GitHub] [beam] y1chi opened a new pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

y1chi opened a new pull request #13933:
URL: https://github.com/apache/beam/pull/13933


   Use vendored cloud build python client so that we can avoid the pain of dealing with third_party dependencies with google internal testing. 
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   Post-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
   --- | --- | --- | --- | --- | --- | ---
   Go | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
   Java | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://ci-beam
 .apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [![Build Status](https://ci-beam.a
 pache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
   Python | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam
 .apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
   XLang | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   --- |Java | Python | Go | Website | Whitespace | Typescript
   --- | --- | --- | --- | --- | --- | ---
   Non-portable | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/be
 am_PreCommit_Go_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
   Portable | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
   
   See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
   
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] y1chi commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-777889887


   friendly ping for another look, @aaltay @tvalentyn 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (875e01e) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **increase** coverage by `0.04%`.
   > The diff coverage is `84.71%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   + Coverage   82.85%   82.89%   +0.04%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    48260     +538     
   - Misses       9874     9957      +83     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <6.89%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
   | [sdks/python/apache\_beam/dataframe/expressions.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL2V4cHJlc3Npb25zLnB5) | `91.02% <0.00%> (-0.14%)` | :arrow_down: |
   | [.../runners/portability/fn\_api\_runner/translations.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3RyYW5zbGF0aW9ucy5weQ==) | `92.10% <0.00%> (-0.08%)` | :arrow_down: |
   | [setup.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
   | [...on/apache\_beam/runners/dataflow/dataflow\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9kYXRhZmxvd19ydW5uZXIucHk=) | `82.28% <0.00%> (ø)` | |
   | [...apache\_beam/runners/portability/portable\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9wb3J0YWJsZV9ydW5uZXIucHk=) | `76.30% <0.00%> (ø)` | |
   | ... and [6 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...875e01e](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] tvalentyn commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
tvalentyn commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776359881


   > I think if they are solely dependencies for google-cloud-build they won't be needed anymore? Do we need some special handling?
   
   Thanks. I thought originally that this PR checks in the google-cloud-build code. Looks like you generated client code with apitools. google-apitools is deprecated, and we were actually trying to move away from depending on google-apitools and use official google-cloud-python libraries. This may be a step in the opposite direction. cc: @aaltay - do you have any opinion here? Perhaps we should look into addressing the difficulty of managing the third_party deps for internal testing instead.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] tvalentyn commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
tvalentyn commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776933914


   Thanks all for the input.
   
   Can we 
   1) add instructions on how to regenerate this client somewhere in the code or documentation, and leave pointers on BEAM-11780? 
   2) move the generated code to apache_beam/io/gcp/internal/clients/
   3) create an (internal) issue for cloudbuild ownernes that documents the friction points?
   
   Thanks!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] aaltay commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776870869


   > > > > I think if they are solely dependencies for google-cloud-build they won't be needed anymore? Do we need some special handling?
   > > > 
   > > > 
   > > > Thanks. I thought originally that this PR checks in the google-cloud-build code. Looks like you generated client code with apitools. google-apitools is deprecated, and we were actually trying to move away from depending on google-apitools and use official google-cloud-python libraries. This may be a step in the opposite direction. cc: @aaltay - do you have any opinion here? Perhaps we should look into addressing the difficulty of managing the third_party deps for internal testing instead.
   > > 
   > > 
   > > > do you have any opinion here?
   > > 
   > > 
   > > This is just an **opinion**: It would be better to avoid generated clients. They do end up causing more issues than regular dependencies, they are hard to upgrade, usually stale, and apitools has been deprecated for a long time. I am fine if we want to do this in there interim and unblock some testing, but a much better solution would be to make this a proper dependency. I do not know the specific issues but this is only one of many dependencies and I assume we will have ways to address the issues with using a dependency.
   > 
   > I think it should be easy to switch back to use the third_party library any time(assuming cloud build team will manage the library internally). The difficulties of using the third party google-cloud-build library are:
   > 
   > * Right now there are dozens of dependencies of google-cloud-build library that are un-managed.
   > * or managed by some other teams but with many customized import hacks.
   > * some dependencies could have been imported to a certain version to be used by certain targets, the version does not satisfy google-cloud-build requirements.
   > 
   > The third party dependency issue is a lot more chaos and hard to control than using google-apitools to generate the client at the moment.
   
   Sounds reasonable to me to proceed with this PR and at the same time ask cloud build team to improve the internal library story.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] tvalentyn merged pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
tvalentyn merged pull request #13933:
URL: https://github.com/apache/beam/pull/13933


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (333940b) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **increase** coverage by `0.04%`.
   > The diff coverage is `84.71%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   + Coverage   82.85%   82.89%   +0.04%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    48260     +538     
   - Misses       9874     9957      +83     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <6.89%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [.../python/apache\_beam/transforms/periodicsequence.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHJhbnNmb3Jtcy9wZXJpb2RpY3NlcXVlbmNlLnB5) | `96.49% <0.00%> (-1.76%)` | :arrow_down: |
   | [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
   | [...ks/python/apache\_beam/runners/worker/sdk\_worker.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc2RrX3dvcmtlci5weQ==) | `89.54% <0.00%> (-0.16%)` | :arrow_down: |
   | [sdks/python/apache\_beam/dataframe/expressions.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL2V4cHJlc3Npb25zLnB5) | `91.02% <0.00%> (-0.14%)` | :arrow_down: |
   | [.../runners/portability/fn\_api\_runner/translations.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3RyYW5zbGF0aW9ucy5weQ==) | `92.10% <0.00%> (-0.08%)` | :arrow_down: |
   | [setup.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
   | ... and [9 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...333940b](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] tvalentyn commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
tvalentyn commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-777930614


   Thanks, @y1chi!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] y1chi edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
y1chi edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776327965


   > 1. What is the process to update or regenerate the GCB code? How does one find out What version of GCB used?
   
   we can use 
   ```
   pip install google-apitools
   
   gen_client --discovery_url=cloudbuild.v1 --overwrite --outdir=third_party/py/apache_beam/runners/dataflow/internal/clients/cloudbuild --root_package=. client
   ```
   to generate the client.
   
   I think the version is only bind to the api url version, in this case we are using cloudbuild v1  https://github.com/apache/beam/blob/875e01e45d5bffbb06bde2b92c60c9dec1c383dd/sdks/python/apache_beam/runners/dataflow/internal/clients/cloudbuild/cloudbuild_v1_client.py#L40. If client is updated it will become v1b1, v1b2 etc. 
   > 2. How do we account for GCB dependencies since top-level library is no longer installed ? I am seeing following deps:
   > 
   > ```
   > google-cloud-build==2.0.0
   >   - google-api-core [required: >=1.22.0,<2.0.0dev, installed: 1.26.0]
   >   - libcst [required: >=0.2.5, installed: 0.3.17]
   >   - proto-plus [required: >=0.4.0, installed: 1.13.0]
   > ```
   
   I think if they are solely dependencies for google-cloud-build they won't be needed anymore? Do we need some special handling?
   
   > 
   > It seems strange that we are modifying the external library for internal testing purposes. Can these or similar changes be done upon import instead?
   
   It will add a lot of hacks into the importing process and I'm guessing we were avoiding that before since there are also other gcp client vendored:
   - apache_beam/runners/dataflow/internal/clients/dataflow
   - apache_beam/io/gcp/internal/clients/bigquery
   - apache_beam/io/gcp/internal/clients/storage
   
   I think it's desirable to use the third_party libraries, but it seems to cause a lot of frictions since the dependencies are often out of sync and have different hacks with copybara internally.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (333940b) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **increase** coverage by `0.04%`.
   > The diff coverage is `84.71%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   + Coverage   82.85%   82.89%   +0.04%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    48260     +538     
   - Misses       9874     9957      +83     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <6.89%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [.../python/apache\_beam/transforms/periodicsequence.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHJhbnNmb3Jtcy9wZXJpb2RpY3NlcXVlbmNlLnB5) | `96.49% <0.00%> (-1.76%)` | :arrow_down: |
   | [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
   | [...ks/python/apache\_beam/runners/worker/sdk\_worker.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc2RrX3dvcmtlci5weQ==) | `89.54% <0.00%> (-0.16%)` | :arrow_down: |
   | [sdks/python/apache\_beam/dataframe/expressions.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL2V4cHJlc3Npb25zLnB5) | `91.02% <0.00%> (-0.14%)` | :arrow_down: |
   | [.../runners/portability/fn\_api\_runner/translations.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3RyYW5zbGF0aW9ucy5weQ==) | `92.10% <0.00%> (-0.08%)` | :arrow_down: |
   | [setup.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
   | ... and [9 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...333940b](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (b53ab1c) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **decrease** coverage by `0.47%`.
   > The diff coverage is `84.85%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   - Coverage   82.85%   82.37%   -0.48%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    47958     +236     
   - Misses       9874    10259     +385     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <7.14%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [...ks/python/apache\_beam/coders/coders\_test\_common.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vY29kZXJzL2NvZGVyc190ZXN0X2NvbW1vbi5weQ==) | `27.30% <0.00%> (-71.22%)` | :arrow_down: |
   | [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `72.93% <0.00%> (-9.78%)` | :arrow_down: |
   | [sdks/python/apache\_beam/coders/coder\_impl.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vY29kZXJzL2NvZGVyX2ltcGwucHk=) | `88.05% <0.00%> (-7.33%)` | :arrow_down: |
   | [sdks/python/apache\_beam/coders/coders.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vY29kZXJzL2NvZGVycy5weQ==) | `82.13% <0.00%> (-5.13%)` | :arrow_down: |
   | [sdks/python/apache\_beam/internal/pickler.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvcGlja2xlci5weQ==) | `82.73% <0.00%> (-3.60%)` | :arrow_down: |
   | [...s/python/apache\_beam/runners/portability/stager.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zdGFnZXIucHk=) | `81.46% <0.00%> (-1.16%)` | :arrow_down: |
   | ... and [13 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...875e01e](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (875e01e) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **increase** coverage by `0.04%`.
   > The diff coverage is `84.71%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   + Coverage   82.85%   82.89%   +0.04%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    48260     +538     
   - Misses       9874     9957      +83     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <6.89%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
   | [sdks/python/apache\_beam/dataframe/expressions.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL2V4cHJlc3Npb25zLnB5) | `91.02% <0.00%> (-0.14%)` | :arrow_down: |
   | [.../runners/portability/fn\_api\_runner/translations.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3RyYW5zbGF0aW9ucy5weQ==) | `92.10% <0.00%> (-0.08%)` | :arrow_down: |
   | [setup.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
   | [...on/apache\_beam/runners/dataflow/dataflow\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9kYXRhZmxvd19ydW5uZXIucHk=) | `82.28% <0.00%> (ø)` | |
   | [...apache\_beam/runners/portability/portable\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9wb3J0YWJsZV9ydW5uZXIucHk=) | `76.30% <0.00%> (ø)` | |
   | ... and [6 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...333940b](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (b53ab1c) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **decrease** coverage by `0.47%`.
   > The diff coverage is `84.85%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   - Coverage   82.85%   82.37%   -0.48%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    47958     +236     
   - Misses       9874    10259     +385     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <7.14%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [...ks/python/apache\_beam/coders/coders\_test\_common.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vY29kZXJzL2NvZGVyc190ZXN0X2NvbW1vbi5weQ==) | `27.30% <0.00%> (-71.22%)` | :arrow_down: |
   | [...ython/apache\_beam/io/gcp/tests/bigquery\_matcher.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL3Rlc3RzL2JpZ3F1ZXJ5X21hdGNoZXIucHk=) | `72.93% <0.00%> (-9.78%)` | :arrow_down: |
   | [sdks/python/apache\_beam/coders/coder\_impl.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vY29kZXJzL2NvZGVyX2ltcGwucHk=) | `88.05% <0.00%> (-7.33%)` | :arrow_down: |
   | [sdks/python/apache\_beam/coders/coders.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vY29kZXJzL2NvZGVycy5weQ==) | `82.13% <0.00%> (-5.13%)` | :arrow_down: |
   | [sdks/python/apache\_beam/internal/pickler.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvcGlja2xlci5weQ==) | `82.73% <0.00%> (-3.60%)` | :arrow_down: |
   | [...s/python/apache\_beam/runners/portability/stager.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zdGFnZXIucHk=) | `81.46% <0.00%> (-1.16%)` | :arrow_down: |
   | ... and [13 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...36bcc50](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] y1chi commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776461701


   > > > I think if they are solely dependencies for google-cloud-build they won't be needed anymore? Do we need some special handling?
   > > 
   > > 
   > > Thanks. I thought originally that this PR checks in the google-cloud-build code. Looks like you generated client code with apitools. google-apitools is deprecated, and we were actually trying to move away from depending on google-apitools and use official google-cloud-python libraries. This may be a step in the opposite direction. cc: @aaltay - do you have any opinion here? Perhaps we should look into addressing the difficulty of managing the third_party deps for internal testing instead.
   > 
   > > do you have any opinion here?
   > 
   > This is just an **opinion**: It would be better to avoid generated clients. They do end up causing more issues than regular dependencies, they are hard to upgrade, usually stale, and apitools has been deprecated for a long time. I am fine if we want to do this in there interim and unblock some testing, but a much better solution would be to make this a proper dependency. I do not know the specific issues but this is only one of many dependencies and I assume we will have ways to address the issues with using a dependency.
   
   I think it should be easy to switch back to use the third_party library any time(assuming cloud build team will manage the library internally). The difficulties of using the third party google-cloud-build library are:
   - Right now there are dozens of dependencies of google-cloud-build library that are un-managed.
   - or managed by some other teams but with many customized import hacks. 
   - some dependencies could have been imported to a certain version to be used by certain targets, the version does not satisfy google-cloud-build requirements.
   
   The third party dependency issue is a lot more chaos and hard to control than using google-apitools to generate the client at the moment.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] y1chi commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776111197


   R: @tvalentyn @emilymye 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] y1chi edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
y1chi edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776327965


   > 1. What is the process to update or regenerate the GCB code? How does one find out What version of GCB used?
   
   we can use 
   ```
   pip install google-apitools
   
   gen_client --discovery_url=cloudbuild.v1 --overwrite --outdir=apache_beam/runners/dataflow/internal/clients/cloudbuild --root_package=. client
   ```
   to generate the client.
   
   I think the version is only bind to the api url version, in this case we are using cloudbuild v1  https://github.com/apache/beam/blob/875e01e45d5bffbb06bde2b92c60c9dec1c383dd/sdks/python/apache_beam/runners/dataflow/internal/clients/cloudbuild/cloudbuild_v1_client.py#L40. If client is updated it will become v1b1, v1b2 etc. 
   > 2. How do we account for GCB dependencies since top-level library is no longer installed ? I am seeing following deps:
   > 
   > ```
   > google-cloud-build==2.0.0
   >   - google-api-core [required: >=1.22.0,<2.0.0dev, installed: 1.26.0]
   >   - libcst [required: >=0.2.5, installed: 0.3.17]
   >   - proto-plus [required: >=0.4.0, installed: 1.13.0]
   > ```
   
   I think if they are solely dependencies for google-cloud-build they won't be needed anymore? Do we need some special handling?
   
   > 
   > It seems strange that we are modifying the external library for internal testing purposes. Can these or similar changes be done upon import instead?
   
   It will add a lot of hacks into the importing process and I'm guessing we were avoiding that before since there are also other gcp client vendored:
   - apache_beam/runners/dataflow/internal/clients/dataflow
   - apache_beam/io/gcp/internal/clients/bigquery
   - apache_beam/io/gcp/internal/clients/storage
   
   I think it's desirable to use the third_party libraries, but it seems to cause a lot of frictions since the dependencies are often out of sync and have different hacks with copybara internally.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] aaltay commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776383961


   > > I think if they are solely dependencies for google-cloud-build they won't be needed anymore? Do we need some special handling?
   > 
   > Thanks. I thought originally that this PR checks in the google-cloud-build code. Looks like you generated client code with apitools. google-apitools is deprecated, and we were actually trying to move away from depending on google-apitools and use official google-cloud-python libraries. This may be a step in the opposite direction. cc: @aaltay - do you have any opinion here? Perhaps we should look into addressing the difficulty of managing the third_party deps for internal testing instead.
   
   > do you have any opinion here? 
   
   This is just an **opinion**: It would be better to avoid generated clients. They do end up causing more issues than regular dependencies, they are hard to upgrade, usually stale, and apitools has been deprecated for a long time. I am fine if we want to do this in there interim and unblock some testing, but a much better solution would be to make this a proper dependency. I do not know the specific issues but this is only one of many dependencies and I assume we will have ways to address the issues with using a dependency.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (875e01e) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **increase** coverage by `0.04%`.
   > The diff coverage is `84.71%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   + Coverage   82.85%   82.89%   +0.04%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    48260     +538     
   - Misses       9874     9957      +83     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <6.89%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
   | [sdks/python/apache\_beam/dataframe/expressions.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL2V4cHJlc3Npb25zLnB5) | `91.02% <0.00%> (-0.14%)` | :arrow_down: |
   | [.../runners/portability/fn\_api\_runner/translations.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3RyYW5zbGF0aW9ucy5weQ==) | `92.10% <0.00%> (-0.08%)` | :arrow_down: |
   | [setup.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
   | [...on/apache\_beam/runners/dataflow/dataflow\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9kYXRhZmxvd19ydW5uZXIucHk=) | `82.28% <0.00%> (ø)` | |
   | [...apache\_beam/runners/portability/portable\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9wb3J0YWJsZV9ydW5uZXIucHk=) | `76.30% <0.00%> (ø)` | |
   | ... and [6 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...875e01e](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] tvalentyn commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
tvalentyn commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776309945


   1) What is the process to update or regenerate the GCB code? How does one find out What version of GCB used?
   2) How do we account for GCB dependencies since top-level library is no longer installed ? I am seeing following deps:
   
   ```
   google-cloud-build==2.0.0
     - google-api-core [required: >=1.22.0,<2.0.0dev, installed: 1.26.0]
     - libcst [required: >=0.2.5, installed: 0.3.17]
     - proto-plus [required: >=0.4.0, installed: 1.13.0]
   ```
   
   It seems strange that we are modifying the external library for internal testing purposes. Can these or similar changes be done upon import instead?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] edited a comment on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-775632335


   # [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=h1) Report
   > Merging [#13933](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=desc) (875e01e) into [master](https://codecov.io/gh/apache/beam/commit/123901f90404022dcb8887173f3d356aff5b1e5d?el=desc) (123901f) will **increase** coverage by `0.04%`.
   > The diff coverage is `84.71%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/beam/pull/13933/graphs/tree.svg?width=650&height=150&src=pr&token=qcbbAh8Fj1)](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13933      +/-   ##
   ==========================================
   + Coverage   82.85%   82.89%   +0.04%     
   ==========================================
     Files         466      469       +3     
     Lines       57596    58217     +621     
   ==========================================
   + Hits        47722    48260     +538     
   - Misses       9874     9957      +83     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...\_beam/runners/portability/sdk\_container\_builder.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9zZGtfY29udGFpbmVyX2J1aWxkZXIucHk=) | `40.21% <6.89%> (-3.07%)` | :arrow_down: |
   | [...nternal/clients/cloudbuild/cloudbuild\_v1\_client.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9jbGllbnQucHk=) | `55.17% <55.17%> (ø)` | |
   | [...s/dataflow/internal/clients/cloudbuild/\_\_init\_\_.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvX19pbml0X18ucHk=) | `80.00% <80.00%> (ø)` | |
   | [...ernal/clients/cloudbuild/cloudbuild\_v1\_messages.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9pbnRlcm5hbC9jbGllbnRzL2Nsb3VkYnVpbGQvY2xvdWRidWlsZF92MV9tZXNzYWdlcy5weQ==) | `100.00% <100.00%> (ø)` | |
   | [sdks/python/apache\_beam/internal/metrics/metric.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW50ZXJuYWwvbWV0cmljcy9tZXRyaWMucHk=) | `86.45% <0.00%> (-1.05%)` | :arrow_down: |
   | [sdks/python/apache\_beam/dataframe/expressions.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vZGF0YWZyYW1lL2V4cHJlc3Npb25zLnB5) | `91.02% <0.00%> (-0.14%)` | :arrow_down: |
   | [.../runners/portability/fn\_api\_runner/translations.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9mbl9hcGlfcnVubmVyL3RyYW5zbGF0aW9ucy5weQ==) | `92.10% <0.00%> (-0.08%)` | :arrow_down: |
   | [setup.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2V0dXAucHk=) | `0.00% <0.00%> (ø)` | |
   | [...on/apache\_beam/runners/dataflow/dataflow\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9kYXRhZmxvdy9kYXRhZmxvd19ydW5uZXIucHk=) | `82.28% <0.00%> (ø)` | |
   | [...apache\_beam/runners/portability/portable\_runner.py](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9wb3J0YWJpbGl0eS9wb3J0YWJsZV9ydW5uZXIucHk=) | `76.30% <0.00%> (ø)` | |
   | ... and [6 more](https://codecov.io/gh/apache/beam/pull/13933/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=footer). Last update [123901f...875e01e](https://codecov.io/gh/apache/beam/pull/13933?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] y1chi commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776957062


   > Thanks all for the input.
   > 
   > Can we
   > 
   > 1. add instructions on how to regenerate this client somewhere in the code or documentation, and leave pointers on [BEAM-11780](https://issues.apache.org/jira/browse/BEAM-11780)?
   
   Done.
   > 2. move the generated code to apache_beam/io/gcp/internal/clients/
   
   This library is purely used in dataflow runner and has no io functionality, doesn't it make more sense to put it under apache_beam/runners/dataflow/internal/clients?
   
   > 3. create an (internal) issue for cloudbuild ownernes that documents the friction points?
   > 
   
   Created b/179919397 as a FR for cloud build team.
   
   > Thanks!
   
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] y1chi commented on pull request #13933: [BEAM-11780] Use vendored cloudbuild python client.

Posted by GitBox <gi...@apache.org>.
y1chi commented on pull request #13933:
URL: https://github.com/apache/beam/pull/13933#issuecomment-776327965


   > 1. What is the process to update or regenerate the GCB code? How does one find out What version of GCB used?
   
   we can use 
   ```
   pip install google-apitools
   
   gen_client --discovery_url=cloudbuild.v1 --overwrite --outdir=third_party/py/apache_beam/runners/dataflow/internal/clients/cloudbuild --root_package=. client
   ```
   to generate the client.
   
   I think the version is only bind to the api url version, in this case we are using cloudbuild v1  https://github.com/apache/beam/blob/875e01e45d5bffbb06bde2b92c60c9dec1c383dd/sdks/python/apache_beam/runners/dataflow/internal/clients/cloudbuild/cloudbuild_v1_client.py#L40. If client is updated it will become v1b1, v1b2 etc. 
   > 2. How do we account for GCB dependencies since top-level library is no longer installed ? I am seeing following deps:
   > 
   > ```
   > google-cloud-build==2.0.0
   >   - google-api-core [required: >=1.22.0,<2.0.0dev, installed: 1.26.0]
   >   - libcst [required: >=0.2.5, installed: 0.3.17]
   >   - proto-plus [required: >=0.4.0, installed: 1.13.0]
   > ```
   
   I think if they are solely dependencies for google-cloud-build they won't be needed anymore? Do we need some special handling?
   
   > 
   > It seems strange that we are modifying the external library for internal testing purposes. Can these or similar changes be done upon import instead?
   
   It will add a lot of hacks into the importing process and I'm guessing we were avoiding that before since they are also other gcp client vendored:
   - apache_beam/runners/dataflow/internal/clients/dataflow
   - apache_beam/io/gcp/internal/clients/bigquery
   - apache_beam/io/gcp/internal/clients/storage
   
   I think it's desirable to use the third_party libraries, but it seems to cause a lot of frictions since the dependencies are often out of sync and have different hacks with copybara internally.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org