weiqingy commented on code in PR #921:
URL: https://github.com/apache/flink-agents/pull/921#discussion_r3624726167
##########
python/flink_agents/plan/actions/action.py:
##########
@@ -95,11 +95,11 @@ def __custom_deserialize(self) -> "Action":
self["config"][name] = value["value"]
return self
for name, value in config.items():
- try:
+ if isinstance(value, list | tuple):
Review Comment:
The serializer only wraps `BaseModel` values into the `(module, class,
value)` shape. A plain list/tuple config value is written as-is and comes back
from JSON as a plain list, so this gate can't tell a serialized model apart
from a genuine list. For `config={"tags": ["a", "b"]}` the round-trip would
reach `import_module("a")` and raise, where the old broad-except kept the raw
list. The java branch above avoids this by tagging wrapped values with
`@class`/`value` and testing membership first.
I'm not sure a plain list/tuple config value is actually reachable today, so
genuinely a question: do we expect list-valued config entries (stop sequences,
tags, tool lists)? If so, would a self-describing marker on the python shape,
mirroring the java tagging, let the deserializer match on the marker instead of
guessing from type, with a round-trip test to lock it in? If plain lists are
out of scope here, feel free to ignore.
--
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]
