alnzng commented on code in PR #921:
URL: https://github.com/apache/flink-agents/pull/921#discussion_r3632518978
##########
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:
@weiqingy Very good question on plain list/tuple config value!
My first thinking was the type check is enough, because normally the model
config comes from our own code. That is why I started with the shape check.
But I checked again and your worry is real, even more than "future".
`add_action(**config)` is public and takes any keyword value, so a user can
already pass `agent.add_action(name=..., func=..., tags=["a", "b"])` today.
In that case the old shape check will treat `["a", "b"]` as a model and call
`import_module("a")`, then it breaks. So it is not only a future assumption, it
is already reachable now.
So I agree `marker` is the better way. When we serialize we tag the value
clearly, and when we deserialize we only rebuild the entries that have the
`marker`. A plain `list` stays a plain `list`. It also keeps `python`
consistent with `java`, which already tags the wrapped value with
`@class`/`value` and checks the `marker` first. Now both sides use the same
way, not two different logics.
For compatibility I think there is no problem:
- The plan is built fresh from the code on every submission and shipped
inside the job graph, not restored from checkpoint, so a new `deserialize`
never meets an old-format plan.
- On `java` side the `python` config path is a generic passthrough (it
writes each value as-is and rebuilds it back to a `map`), so the `marker` dict
goes through the same as before, no `java` change needed.
Please take a look again on my new changes, thanks.
--
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]
