gemini-code-assist[bot] commented on code in PR #36793: URL: https://github.com/apache/beam/pull/36793#discussion_r2518998270
########## sdks/python/apache_beam/internal/cloudpickle/cloudpickle.py: ########## @@ -1720,3 +1733,10 @@ def dumps( # Backward compat alias. CloudPickler = Pickler + + +def hash_dynamic_classdef(classdef): + """Generates a deterministic ID by hashing the pickled class definition.""" + hexidgest = hashlib.sha256( + dumps(classdef, config=CloudPickleConfig(id_generator=None))).hexdigest() + return hexidgest Review Comment:  There's a typo in the variable name `hexidgest`; it should be `hexdigest`. Additionally, this change removes the trailing newline from the file. According to PEP 8, files should end with a single newline. Please add one. ```suggestion hexdigest = hashlib.sha256( dumps(classdef, config=CloudPickleConfig(id_generator=None))).hexdigest() return hexdigest ``` ########## sdks/python/apache_beam/internal/cloudpickle/cloudpickle.py: ########## @@ -179,10 +181,21 @@ class CloudPickleConfig: def _get_or_create_tracker_id(class_def, id_generator): with _DYNAMIC_CLASS_TRACKER_LOCK: class_tracker_id = _DYNAMIC_CLASS_TRACKER_BY_CLASS.get(class_def) + if class_tracker_id is _GENERATING_SENTINEL and id_generator: + raise RuntimeError( + f"Recursive ID generation detected for {class_def}. " + f"The id_generator cannot recursively request an ID for the same class." + ) + if class_tracker_id is None and id_generator is not None: - class_tracker_id = id_generator(class_def) - _DYNAMIC_CLASS_TRACKER_BY_CLASS[class_def] = class_tracker_id - _DYNAMIC_CLASS_TRACKER_BY_ID[class_tracker_id] = class_def + _DYNAMIC_CLASS_TRACKER_BY_CLASS[class_def] = _GENERATING_SENTINEL + try: + class_tracker_id = id_generator(class_def) + _DYNAMIC_CLASS_TRACKER_BY_CLASS[class_def] = class_tracker_id + _DYNAMIC_CLASS_TRACKER_BY_ID[class_tracker_id] = class_def + except: + _DYNAMIC_CLASS_TRACKER_BY_CLASS.pop(class_def, None) + raise Review Comment:  Using a bare `except:` can catch more exceptions than intended, such as `SystemExit` or `KeyboardInterrupt`, which can hide bugs or make it harder to stop the program. It's better to be more specific and catch `Exception` instead to avoid unintentionally swallowing system-level exceptions. ```suggestion except Exception: _DYNAMIC_CLASS_TRACKER_BY_CLASS.pop(class_def, None) raise ``` -- 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]
