damccorm commented on code in PR #38769:
URL: https://github.com/apache/beam/pull/38769#discussion_r3342226784
##########
sdks/python/apache_beam/ml/transforms/base.py:
##########
@@ -591,7 +592,18 @@ def save_attributes(
def load_attributes(artifact_location):
with FileSystems.open(os.path.join(artifact_location,
_ATTRIBUTE_FILE_NAME),
'rb') as f:
- return jsonpickle.decode(f.read())
+ # load_attributes runs eagerly during MLTransform.expand() at pipeline
+ # construction time, so the pipeline's options are available via the
+ # construction-time context.
+ pipeline_options = get_pipeline_options()
+ safe = True
+ if (pipeline_options is not None and
+ pipeline_options.is_compat_version_prior_to("2.75.0")):
+ # Keep the pre-2.75.0 jsonpickle behavior (safe=False permits
+ # eval-based decoding) for backwards compatibility with already-staged
+ # artifacts.
+ safe = False
Review Comment:
> Is the benefit of always setting to False simplicity? I don't have a
strong opinion here.
Yeah - IMO this is either a vulnerability which we should unconditionally
patch, or it is not and we can retain a single behavior.
> I also think it is reasonable to just skip compat flag completely. I doubt
it will break anyone, and if it does they can file a bug and we can add a
compat flag in an upcoming beam version?
I agree with this. I actually also realized that the compat flag doesn't
work here anyways - these artifacts persist across (usually batch) pipeline
runs, so it isn't coming from an update, its coming from a subsequent pipeline
run.
My slight preference would be `safe=False`, but I'm good with either
approach as long as we skip the compat flag piece and will defer to you here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
