tickets/DM-33992 Fix serialization of ConfigurableActions #649
Conversation
| value = self.__get__(instance) | ||
| fullname = _joinNamePath(instance._name, self.name) | ||
| outfile.write(f"{fullname}={_typeStr(value)}\n") | ||
| super().save(outfile, instance) |
There was a problem hiding this comment.
It looks like this implementation is already doing everything it needs to do, and delegating it to super will just do some of those things again (incorrectly, I assume). Can you add a comment explaining why you're still delegating to super here, and why that doesn't cause the field state to be written again?
There was a problem hiding this comment.
I think I brought this up somewhere (perhaps it was slack) but I think ConfigFields (parent class) are broken in the same way, but I didnt want to make changes to pex_config that could have wider impact w/o investigating it.
ConfigField simply gets its value (the config instance) and calls the ._save method on that. However that method on a Config instance simply loops over the fields declared on that config calling their save method. No where does the Config type itself get assigned in the persisted config, leading to it always defaulting.
There was a problem hiding this comment.
No where does the Config type itself get assigned in the persisted config, leading to it always defaulting.
In the base ConfigField, the type of the config is completely static, so I don't think this is necessarily a problem there, though it obviously is for ConfigurableAction.
| @@ -85,6 +85,8 @@ def makeColumnExpressionAction(className: str, expr: str, | |||
| Type[DataFrameAction]]]] = None, | |||
| docstring: str = None | |||
| ) -> Type[DataFrameAction]: | |||
| import inspect | |||
| new_module = inspect.stack()[1].frame.f_locals['__name__'] | |||
There was a problem hiding this comment.
Eek, is this the kind of thing we do elsewhere in pex_config to get module names?
If so, pex_config has made me sad today. If not, I'm not gonna get in the way, but my degree of caution about using ConfigurableAction more broadly (and hope that someday we'll be able to replace it) will increase.
In either case, could you add a check for this evaluating to __main__ and raise an exception if that happens, since otherwise that'll lead to more confusing later exception?
There was a problem hiding this comment.
I thought about the need to add a comment on this while falling asleep, and forgot by the time I woke up. I will clarify here, this is not a generic ConfigurableAction feature. This function is a factory for creating ConfigurableActions based on an expression defined by a string (see here )
The problem this solves is that the class thinks its namespace resolves to inside the function body when pex_config asks, however the actual import path is the module that calls this factory to produce the action. This is not dissimilar to namedtuples.
Configurable actions (and derived classes) were not being properly serialized into a pex_config override file, which made it impossible to load a previously saved config. This patch ensures the correct values are written to the output file.
natelust
force-pushed
the
tickets/DM-33992
branch
from
c1464f6
to
1e6a4e5
Compare
Configurable actions (and derived classes) were not being properly
serialized into a pex_config override file, which made it impossible
to load a previously saved config. This patch ensures the correct
values are written to the output file.