You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "claudevdm (via GitHub)" <gi...@apache.org> on 2025/10/01 13:55:39 UTC
[PR] Use relpath coders [beam]
claudevdm opened a new pull request, #36345:
URL: https://github.com/apache/beam/pull/36345
**Please** add a meaningful description for your change here
------------------------
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36345:
URL: https://github.com/apache/beam/pull/36345#issuecomment-3357271476
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment `assign set of reviewers`
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2399711175
##########
sdks/python/apache_beam/coders/coders.py:
##########
@@ -1016,9 +1024,22 @@ def _should_force_use_dill():
def _update_compatible_deterministic_fast_primitives_coder(coder, step_label):
- if _should_force_use_dill():
+ """ Returns the update compatible version of DeterministicFastPrimitivesCoder
+ The differences are in how "special types" e.g. NamedTuples, Dataclasses are
+ deterministically encoded.
+
+ - In SDK version <= 2.67.0 dill is used to encode "special types"
+ - In SDK version 2.68.0 cloudpickle is used to encode "special types" with
+ absolute filepaths in code objects and dynamic functions.
+ - In SDK version 2.69.0 cloudpickle is used to encode "special types" with
Review Comment:
and later?
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2399711175
##########
sdks/python/apache_beam/coders/coders.py:
##########
@@ -1016,9 +1024,22 @@ def _should_force_use_dill():
def _update_compatible_deterministic_fast_primitives_coder(coder, step_label):
- if _should_force_use_dill():
+ """ Returns the update compatible version of DeterministicFastPrimitivesCoder
+ The differences are in how "special types" e.g. NamedTuples, Dataclasses are
+ deterministically encoded.
+
+ - In SDK version <= 2.67.0 dill is used to encode "special types"
+ - In SDK version 2.68.0 cloudpickle is used to encode "special types" with
+ absolute filepaths in code objects and dynamic functions.
+ - In SDK version 2.69.0 cloudpickle is used to encode "special types" with
Review Comment:
Should we say: In SDK version 2.69.0 and later?
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2395447247
##########
sdks/python/apache_beam/coders/coders_test_common.py:
##########
@@ -248,12 +249,13 @@ def test_memoizing_pickle_coder(self):
@parameterized.expand([
param(compat_version=None),
param(compat_version="2.67.0"),
+ param(compat_version="2.68.0"),
Review Comment:
Could you please call out somewhere in a comment notable behavior differences in 2.67.0, 2.68.0 that are relevant for this test scenario(s)?
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2399717097
##########
sdks/python/apache_beam/coders/coders.py:
##########
@@ -1016,9 +1024,22 @@ def _should_force_use_dill():
def _update_compatible_deterministic_fast_primitives_coder(coder, step_label):
- if _should_force_use_dill():
+ """ Returns the update compatible version of DeterministicFastPrimitivesCoder
+ The differences are in how "special types" e.g. NamedTuples, Dataclasses are
+ deterministically encoded.
+
+ - In SDK version <= 2.67.0 dill is used to encode "special types"
+ - In SDK version 2.68.0 cloudpickle is used to encode "special types" with
+ absolute filepaths in code objects and dynamic functions.
+ - In SDK version 2.69.0 cloudpickle is used to encode "special types" with
Review Comment:
(also feel free to write the explanatory comment once and reference it later in other docstrings, or move these tests into a separate file if it helps.)
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "claudevdm (via GitHub)" <gi...@apache.org>.
claudevdm commented on PR #36345:
URL: https://github.com/apache/beam/pull/36345#issuecomment-3357268018
R: @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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2395447247
##########
sdks/python/apache_beam/coders/coders_test_common.py:
##########
@@ -248,12 +249,13 @@ def test_memoizing_pickle_coder(self):
@parameterized.expand([
param(compat_version=None),
param(compat_version="2.67.0"),
+ param(compat_version="2.68.0"),
Review Comment:
Could you please call out somewhere in a comment notable behavior differences in 2.67.0, 2.68.0 that are relevant for this test scenario?
##########
sdks/python/apache_beam/coders/coder_impl.py:
##########
@@ -377,12 +378,14 @@ def __init__(
self,
fallback_coder_impl,
requires_deterministic_step_label=None,
- force_use_dill=False):
+ force_use_dill=False,
+ skip_use_relative_filepaths=False):
Review Comment:
in general, i try to avoid double-negations. would it make sense to use
`use_relative_filepaths=True` instead?
##########
sdks/python/apache_beam/coders/coders_test_common.py:
##########
@@ -799,6 +805,17 @@ def run_subprocess():
logging.warning("Could not decode %s data due to %s", test_name, e)
continue
+ if test_name == "named_tuple_simple" and not is_using_dill:
+ should_have_relative_path = not compat_version
Review Comment:
could you add a comment explaining why `should_have_relative_path = not compat_version`
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2395455040
##########
sdks/python/apache_beam/coders/coder_impl.py:
##########
@@ -377,12 +378,14 @@ def __init__(
self,
fallback_coder_impl,
requires_deterministic_step_label=None,
- force_use_dill=False):
+ force_use_dill=False,
+ skip_use_relative_filepaths=False):
Review Comment:
in general, i try to avoid double-negations. would it make sense to use
`use_relative_filepaths=True` instead as the default or `use_absolute_filepaths=True`?
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn merged PR #36345:
URL: https://github.com/apache/beam/pull/36345
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2395455040
##########
sdks/python/apache_beam/coders/coder_impl.py:
##########
@@ -377,12 +378,14 @@ def __init__(
self,
fallback_coder_impl,
requires_deterministic_step_label=None,
- force_use_dill=False):
+ force_use_dill=False,
+ skip_use_relative_filepaths=False):
Review Comment:
in general, i try to avoid double-negations. would it make sense to use
`use_relative_filepaths=True/False` instead as the default or `use_absolute_filepaths=True`?
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2395455040
##########
sdks/python/apache_beam/coders/coder_impl.py:
##########
@@ -377,12 +378,14 @@ def __init__(
self,
fallback_coder_impl,
requires_deterministic_step_label=None,
- force_use_dill=False):
+ force_use_dill=False,
+ skip_use_relative_filepaths=False):
Review Comment:
in general, i try to avoid double-negations. would it make sense to use
`use_relative_filepaths=True/False` instead as the default or `use_absolute_filepaths`?
--
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
Re: [PR] Pickle the relative path for code objects when cloudpickle is used to encode special deterministic types. [beam]
Posted by "tvalentyn (via GitHub)" <gi...@apache.org>.
tvalentyn commented on code in PR #36345:
URL: https://github.com/apache/beam/pull/36345#discussion_r2399717097
##########
sdks/python/apache_beam/coders/coders.py:
##########
@@ -1016,9 +1024,22 @@ def _should_force_use_dill():
def _update_compatible_deterministic_fast_primitives_coder(coder, step_label):
- if _should_force_use_dill():
+ """ Returns the update compatible version of DeterministicFastPrimitivesCoder
+ The differences are in how "special types" e.g. NamedTuples, Dataclasses are
+ deterministically encoded.
+
+ - In SDK version <= 2.67.0 dill is used to encode "special types"
+ - In SDK version 2.68.0 cloudpickle is used to encode "special types" with
+ absolute filepaths in code objects and dynamic functions.
+ - In SDK version 2.69.0 cloudpickle is used to encode "special types" with
Review Comment:
(also feel free to mention a docstring once and reference it later, or move these tests into a separate file if it helps.)
--
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