You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "damccorm (via GitHub)" <gi...@apache.org> on 2023/08/09 19:50:12 UTC
[GitHub] [beam] damccorm opened a new pull request, #27939: Update how we specify artifact modes
damccorm opened a new pull request, #27939:
URL: https://github.com/apache/beam/pull/27939
Using read_artifact_location and write_artifact_location seems cleaner. This doesn't extend down to the TFTProcessHandler since that's not user facing, though that would be a nice future cleanup.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] 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://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-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)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+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 or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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] robertwb commented on a diff in pull request #27939: Update how we specify artifact modes
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #27939:
URL: https://github.com/apache/beam/pull/27939#discussion_r1289144166
##########
sdks/python/apache_beam/ml/transforms/base.py:
##########
@@ -115,8 +115,8 @@ class MLTransform(beam.PTransform[beam.PCollection[ExampleT],
def __init__(
self,
*,
- artifact_location: str,
- artifact_mode: str = ArtifactMode.PRODUCE,
+ write_artifact_location: str = '',
Review Comment:
Or would `Optional[str] = None` be preferable?
##########
sdks/python/apache_beam/ml/transforms/base.py:
##########
@@ -143,23 +143,43 @@ def __init__(
retrieved from this location. Note that when consuming artifacts,
it is not necessary to pass the transforms since they are inherently
stored within the artifacts themselves. The value assigned to
- `artifact_location` should be a valid storage path where the artifacts
- can be written to or read from.
+ `write_artifact_location` should be a valid storage path where the
+ artifacts can be written to. Only one of write_artifact_location and
+ read_artifact_location should be specified.
+ read_artifact_location: A storage location to read artifacts resulting
+ froma previous MLTransform. These artifacts include transformations
+ applied to the dataset and generated values like min, max from
+ ScaleTo01, and mean, var from ScaleToZScore. Note that when consuming
+ artifacts, it is not necessary to pass the transforms since they are
+ inherently stored within the artifacts themselves. The value assigned
+ to `read_artifact_location` should be a valid storage path where the
+ artifacts can be read from. Only one of write_artifact_location and
+ read_artifact_location should be specified.
transforms: A list of transforms to apply to the data. All the transforms
are applied in the order they are specified. The input of the
i-th transform is the output of the (i-1)-th transform. Multi-input
transforms are not supported yet.
- artifact_mode: Whether to produce or consume artifacts. If set to
- 'consume', MLTransform will assume that the artifacts are already
- computed and stored in the artifact_location. Pass the same artifact
- location that was passed during produce phase to ensure that the
- right artifacts are read. If set to 'produce', MLTransform
- will compute the artifacts and store them in the artifact_location.
- The artifacts will be read from this location during the consume phase.
"""
if transforms:
_ = [self._validate_transform(transform) for transform in transforms]
+ if len(read_artifact_location) > 0 and len(write_artifact_location) > 0:
Review Comment:
Regardless of the comment above, I think `if read_artifact_location and write_artifact_location:` reads clearer. Similarly below one can write `if not read_artifact_location and not write_artifact_location` (or, equivalently `if not (read_artifact_location or write_artifact_location)`).
--
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 #27939: Update how we specify artifact modes
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #27939:
URL: https://github.com/apache/beam/pull/27939#issuecomment-1672075332
## [Codecov](https://app.codecov.io/gh/apache/beam/pull/27939?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#27939](https://app.codecov.io/gh/apache/beam/pull/27939?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6475afe) into [master](https://app.codecov.io/gh/apache/beam/commit/fd6040d2141db010e07edcb7113e8e69fcabdaa0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (fd6040d) will **decrease** coverage by `0.01%`.
> Report is 2 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #27939 +/- ##
==========================================
- Coverage 70.58% 70.57% -0.01%
==========================================
Files 857 857
Lines 103947 103956 +9
==========================================
+ Hits 73367 73370 +3
- Misses 29020 29026 +6
Partials 1560 1560
```
| Flag | Coverage Δ | |
|---|---|---|
| python | `79.54% <0.00%> (-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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files Changed](https://app.codecov.io/gh/apache/beam/pull/27939?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [sdks/python/apache\_beam/ml/transforms/base.py](https://app.codecov.io/gh/apache/beam/pull/27939?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvdHJhbnNmb3Jtcy9iYXNlLnB5) | `56.45% <0.00%> (-9.59%)` | :arrow_down: |
| [sdks/python/apache\_beam/ml/transforms/handlers.py](https://app.codecov.io/gh/apache/beam/pull/27939?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c2Rrcy9weXRob24vYXBhY2hlX2JlYW0vbWwvdHJhbnNmb3Jtcy9oYW5kbGVycy5weQ==) | `5.97% <ø> (ø)` | |
... and [6 files with indirect coverage changes](https://app.codecov.io/gh/apache/beam/pull/27939/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] damccorm commented on pull request #27939: Update how we specify artifact modes
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on PR #27939:
URL: https://github.com/apache/beam/pull/27939#issuecomment-1672056854
R: @robertwb
--
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] damccorm commented on pull request #27939: Update how we specify artifact modes
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm commented on PR #27939:
URL: https://github.com/apache/beam/pull/27939#issuecomment-1672060385
FYI @AnandInguva
--
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] github-actions[bot] commented on pull request #27939: Update how we specify artifact modes
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #27939:
URL: https://github.com/apache/beam/pull/27939#issuecomment-1672058101
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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] damccorm merged pull request #27939: Update how we specify artifact modes
Posted by "damccorm (via GitHub)" <gi...@apache.org>.
damccorm merged PR #27939:
URL: https://github.com/apache/beam/pull/27939
--
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] robertwb commented on a diff in pull request #27939: Update how we specify artifact modes
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #27939:
URL: https://github.com/apache/beam/pull/27939#discussion_r1289167487
##########
sdks/python/apache_beam/ml/transforms/base.py:
##########
@@ -143,23 +143,46 @@ def __init__(
retrieved from this location. Note that when consuming artifacts,
it is not necessary to pass the transforms since they are inherently
stored within the artifacts themselves. The value assigned to
- `artifact_location` should be a valid storage path where the artifacts
- can be written to or read from.
+ `write_artifact_location` should be a valid storage directory where the
+ artifacts from this transform can be written to. If no directory exists
+ at this location, one will be created. This will overwrite any
+ artifacts already in this location, so distinct locations should be
+ used for each instance of MLTransform. Only one of
+ write_artifact_location and read_artifact_location should be specified.
+ read_artifact_location: A storage location to read artifacts resulting
+ froma previous MLTransform. These artifacts include transformations
+ applied to the dataset and generated values like min, max from
+ ScaleTo01, and mean, var from ScaleToZScore. Note that when consuming
+ artifacts, it is not necessary to pass the transforms since they are
+ inherently stored within the artifacts themselves. The value assigned
+ to `read_artifact_location` should be a valid storage path where the
+ artifacts can be read from. Only one of write_artifact_location and
+ read_artifact_location should be specified.
transforms: A list of transforms to apply to the data. All the transforms
are applied in the order they are specified. The input of the
i-th transform is the output of the (i-1)-th transform. Multi-input
transforms are not supported yet.
- artifact_mode: Whether to produce or consume artifacts. If set to
- 'consume', MLTransform will assume that the artifacts are already
- computed and stored in the artifact_location. Pass the same artifact
- location that was passed during produce phase to ensure that the
- right artifacts are read. If set to 'produce', MLTransform
- will compute the artifacts and store them in the artifact_location.
- The artifacts will be read from this location during the consume phase.
"""
if transforms:
_ = [self._validate_transform(transform) for transform in transforms]
+ if read_artifact_location and write_artifact_location:
+ raise ValueError(
+ 'Only one of read_artifact_location or write_artifact_location can '
+ 'be specified to initialize MLTransform')
+
+ if not read_artifact_location and not write_artifact_location:
+ raise ValueError(
+ 'Either a read_artifact_location or write_artifact_location must be '
+ 'specified to initialize MLTransform')
+
+ artifact_location = write_artifact_location
+ artifact_mode = ArtifactMode.PRODUCE
+
+ if len(read_artifact_location) > 0:
Review Comment:
Just noticed this will be an error with None
I prefer to avoid the write-then-overwrite pattern. E.g. I'd do something like
```
if read_artifact_location and write_artifact_location:
[error]
elif write_artifact_location:
artifact_location = write_artifact_location
artifact_mode = ArtifactMode.PRODUCE
elif read_artifact_location:
artifact_location = read_artifact_location
artifact_mode = ArtifactMode.CONSUME
else:
error that at least one must be provided.
```
This does of course move a bit of logic up into the argument checking. One can also do
```
if write_artifact_location:
artifact_location = write_artifact_location
artifact_mode = ArtifactMode.PRODUCE
else read_artifact_location:
artifact_location = read_artifact_location
artifact_mode = ArtifactMode.CONSUME
```
which assumes the preconditions are met.
--
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] robertwb commented on a diff in pull request #27939: Update how we specify artifact modes
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #27939:
URL: https://github.com/apache/beam/pull/27939#discussion_r1289149574
##########
sdks/python/apache_beam/ml/transforms/base.py:
##########
@@ -134,7 +134,7 @@ def __init__(
themselves.
Args:
- artifact_location: A storage location for artifacts resulting from
+ write_artifact_location: A storage location for artifacts resulting from
Review Comment:
Maybe add a note that this will overwrite any other artifacts in this location, and distinct artifact locations should be used for distinct ML transforms?
##########
sdks/python/apache_beam/ml/transforms/base.py:
##########
@@ -134,7 +134,7 @@ def __init__(
themselves.
Args:
- artifact_location: A storage location for artifacts resulting from
+ write_artifact_location: A storage location for artifacts resulting from
Review Comment:
Maybe mention that this should be a directory, and that it will be created if it does not already exist?
--
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