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 2022/04/24 02:59:26 UTC
[GitHub] [beam] BigJerBD opened a new pull request, #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
BigJerBD opened a new pull request, #17455:
URL: https://github.com/apache/beam/pull/17455
Improvement to resolve : https://issues.apache.org/jira/browse/BEAM-13250
first commit is identical to https://github.com/apache/beam/pull/15977, but since I believe this is relevant , I am open to put some effort to integrate a proper solution
This change is relevant for testing purpose. Being able to change `storage_client` allows use gcs emulator or other gcs configurations. (for example: https://github.com/fsouza/fake-gcs-server/issues/623#issuecomment-1107093547 )
This way it is possible at the very least to do :
```
class TestGCSFileSystem(GCSFileSystem):
GCS_PREFIX = 'gs-test://'
@classmethod
def scheme(cls):
return 'gs-test'
@staticmethod
def get_gcsio():
return gcsio.GcsIO(
storage_client=storage.StorageV1(
url="http://my_custom_url...",
# Other custom configurations ...
)
)
```
Please note that this is not only usefull for testing purpose, it allows more flexibility to configure the storage_client for the price of adding another user defined filesystem scheme
Another alternative could to instead add an beam options to be able to replace the url or other options. This might be better since it does not a new function to the GcsFileSystem that could be used by sdk users
Thanks :)
R: @aaltay @ihji
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] 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).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
aaltay commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1112545185
R: @johnjcasey
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1107693154
Can one of the admins verify this patch?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1107693151
Can one of the admins verify this patch?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] codecov[bot] commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1107697521
# [Codecov](https://codecov.io/gh/apache/beam/pull/17455?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#17455](https://codecov.io/gh/apache/beam/pull/17455?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c34cd42) into [master](https://codecov.io/gh/apache/beam/commit/3f2e3c7c9eccb9d40370cbc70e9a451a4b5573f5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3f2e3c7) will **decrease** coverage by `0.00%`.
> The diff coverage is `85.71%`.
```diff
@@ Coverage Diff @@
## master #17455 +/- ##
==========================================
- Coverage 73.92% 73.92% -0.01%
==========================================
Files 689 689
Lines 90397 90400 +3
==========================================
- Hits 66829 66827 -2
- Misses 22384 22389 +5
Partials 1184 1184
```
| Flag | Coverage Δ | |
|---|---|---|
| python | `83.64% <85.71%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/beam/pull/17455?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [sdks/python/apache\_beam/io/gcp/gcsfilesystem.py](https://codecov.io/gh/apache/beam/pull/17455/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vaW8vZ2NwL2djc2ZpbGVzeXN0ZW0ucHk=) | `90.22% <85.71%> (+0.22%)` | :arrow_up: |
| [sdks/python/apache\_beam/utils/interactive\_utils.py](https://codecov.io/gh/apache/beam/pull/17455/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdXRpbHMvaW50ZXJhY3RpdmVfdXRpbHMucHk=) | `87.80% <0.00%> (-7.32%)` | :arrow_down: |
| [...eam/runners/interactive/interactive\_environment.py](https://codecov.io/gh/apache/beam/pull/17455/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy9pbnRlcmFjdGl2ZS9pbnRlcmFjdGl2ZV9lbnZpcm9ubWVudC5weQ==) | `90.18% <0.00%> (-0.31%)` | :arrow_down: |
| [...hon/apache\_beam/runners/worker/bundle\_processor.py](https://codecov.io/gh/apache/beam/pull/17455/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvYnVuZGxlX3Byb2Nlc3Nvci5weQ==) | `93.39% <0.00%> (-0.25%)` | :arrow_down: |
| [sdks/python/apache\_beam/transforms/util.py](https://codecov.io/gh/apache/beam/pull/17455/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHJhbnNmb3Jtcy91dGlsLnB5) | `95.98% <0.00%> (-0.17%)` | :arrow_down: |
| [...ks/python/apache\_beam/runners/worker/sdk\_worker.py](https://codecov.io/gh/apache/beam/pull/17455/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vcnVubmVycy93b3JrZXIvc2RrX3dvcmtlci5weQ==) | `89.06% <0.00%> (+0.15%)` | :arrow_up: |
| [.../python/apache\_beam/typehints/trivial\_inference.py](https://codecov.io/gh/apache/beam/pull/17455/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vdHlwZWhpbnRzL3RyaXZpYWxfaW5mZXJlbmNlLnB5) | `96.41% <0.00%> (+0.29%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/beam/pull/17455?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/beam/pull/17455?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3f2e3c7...c34cd42](https://codecov.io/gh/apache/beam/pull/17455?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] BigJerBD commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
BigJerBD commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1126250681
Perfect I'll apply the change in the coming week
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] asf-ci commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
asf-ci commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1107693153
Can one of the admins verify this patch?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] BigJerBD commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
BigJerBD commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1126637509
Closing this PR since a similar change seems to have been merged to master :
currently on master:
```python
# sdks/python/apache_beam/io/gcp/gcsfilesystem.py ...
def _gcsIO(self):
return gcsio.GcsIO(pipeline_options=self._pipeline_options)
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] BigJerBD commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
BigJerBD commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1114294629
This could be done to simplify some tests that patches on the module level
```python
# in sdks/python/apache_beam/io/gcp/gcsfilesystem_test.py
@mock.patch('apache_beam.io.gcp.gcsfilesystem.gcsio') # this could be removed
def test_create(self, mock_gcsio):
# Prepare mocks.
gcsio_mock = mock.MagicMock()
gcsfilesystem.gcsio.GcsIO = lambda: gcsio_mock # this would become: self.fs.get_gcsio = lambda: gcsio_mock
...
```
Would it be enough?
it might looks irrelevant but at the very least sdk users using those classes directly do not have to patch modules to override some gcsio.GcsIO properties if considered necessary
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] johnjcasey commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
johnjcasey commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1126078445
Yep, that would work for me.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
aaltay commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1125443964
@johnjcasey - would you be able to respond to the last question?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] BigJerBD closed pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
BigJerBD closed pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
URL: https://github.com/apache/beam/pull/17455
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] johnjcasey commented on pull request #17455: [BEAM-13250] Factorise gcsio.GcsIO() from gcsfilesystem to allow overriding
Posted by GitBox <gi...@apache.org>.
johnjcasey commented on PR #17455:
URL: https://github.com/apache/beam/pull/17455#issuecomment-1112640903
I don't see anything wrong with this change per se, but I think it would make more sense alongside a test change that leverages the new factorization. Do you have a specific test scenario that would be helped by this?
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org