damccorm commented on code in PR #34310:
URL: https://github.com/apache/beam/pull/34310#discussion_r1998883906
##########
sdks/python/apache_beam/ml/anomaly/specifiable.py:
##########
@@ -104,33 +105,47 @@ class Spec():
config: Optional[Dict[str, Any]] = dataclasses.field(default_factory=dict)
-@runtime_checkable
-class Specifiable(Protocol):
- """Protocol that a specifiable class needs to implement."""
- #: The value of the `type` field in the object's spec for this class.
- spec_type: ClassVar[str]
- #: The raw keyword arguments passed to `__init__` method during object
- #: initialization.
- init_kwargs: dict[str, Any]
+def _from_spec_helper(v, _run_init):
Review Comment:
These should probably be `_specifiable_from_spec_helper` and `_specifiable
_to_spec_helper` now
##########
sdks/python/apache_beam/ml/anomaly/specifiable.py:
##########
@@ -104,33 +105,47 @@ class Spec():
config: Optional[Dict[str, Any]] = dataclasses.field(default_factory=dict)
-@runtime_checkable
-class Specifiable(Protocol):
- """Protocol that a specifiable class needs to implement."""
- #: The value of the `type` field in the object's spec for this class.
- spec_type: ClassVar[str]
- #: The raw keyword arguments passed to `__init__` method during object
- #: initialization.
- init_kwargs: dict[str, Any]
+def _from_spec_helper(v, _run_init):
+ if isinstance(v, Spec):
+ return Specifiable.from_spec(v, _run_init)
+
+ if isinstance(v, List):
+ return [_from_spec_helper(e, _run_init) for e in v]
+
+ return v
+
+
+def _to_spec_helper(v):
+ if isinstance(v, Specifiable):
+ return v.to_spec()
+
+ if isinstance(v, List):
+ return [_to_spec_helper(e) for e in v]
+
+ if inspect.isfunction(v):
+ if not hasattr(v, "spec_type"):
+ _register(v, inject_spec_type=False)
+ return Spec(type=_get_default_spec_type(v), config=None)
- # a boolean to tell whether the original `__init__` method is called
- _initialized: bool
- # a boolean used by new_getattr to tell whether it is in the `__init__`
method
- # call
- _in_init: bool
+ if inspect.isclass(v):
+ if not hasattr(v, "spec_type"):
+ _register(v, inject_spec_type=False)
+ return Spec(type=_get_default_spec_type(v), config=None)
- @staticmethod
- def _from_spec_helper(v, _run_init):
- if isinstance(v, Spec):
- return Specifiable.from_spec(v, _run_init)
+ return v
Review Comment:
Hm, should we actually throw here? Or make that a parameterizable behavior?
There are a lot of types not covered here (e.g. dict), which would now not get
spec treatement.
##########
sdks/python/apache_beam/ml/anomaly/specifiable.py:
##########
@@ -250,13 +250,35 @@ def _get_init_kwargs(inst, init_method, *args, **kwargs):
return params
+@overload
def specifiable(
- my_cls=None,
+ my_cls: None = None,
/,
*,
- spec_type=None,
- on_demand_init=True,
- just_in_time_init=True):
+ spec_type: Optional[str] = None,
+ on_demand_init: bool = True,
+ just_in_time_init: bool = True) -> Callable[[T], T]:
+ ...
+
+
+@overload
Review Comment:
Why do we need these overloads?
##########
sdks/python/apache_beam/ml/anomaly/specifiable.py:
##########
@@ -195,14 +189,19 @@ def to_spec(self) -> Spec:
f"'{type(self).__name__}' not registered as Specifiable. "
f"Decorate ({type(self).__name__}) with @specifiable")
- args = {k: self._to_spec_helper(v) for k, v in self.init_kwargs.items()}
+ args = {k: _to_spec_helper(v) for k, v in self.init_kwargs.items()}
- return Spec(type=self.__class__.spec_type, config=args)
+ return Spec(type=self.spec_type(), config=args)
def run_original_init(self) -> None:
"""Invoke the original __init__ method with original keyword arguments"""
pass
+ @classmethod
+ def unspecifiable(cls) -> None:
+ """Resume the class structure prior to specifiable"""
Review Comment:
I don't quite understand - why do we need this?
##########
sdks/python/apache_beam/ml/anomaly/specifiable.py:
##########
@@ -104,33 +105,47 @@ class Spec():
config: Optional[Dict[str, Any]] = dataclasses.field(default_factory=dict)
-@runtime_checkable
-class Specifiable(Protocol):
- """Protocol that a specifiable class needs to implement."""
- #: The value of the `type` field in the object's spec for this class.
- spec_type: ClassVar[str]
- #: The raw keyword arguments passed to `__init__` method during object
- #: initialization.
- init_kwargs: dict[str, Any]
+def _from_spec_helper(v, _run_init):
+ if isinstance(v, Spec):
+ return Specifiable.from_spec(v, _run_init)
+
+ if isinstance(v, List):
+ return [_from_spec_helper(e, _run_init) for e in v]
+
+ return v
+
+
+def _to_spec_helper(v):
+ if isinstance(v, Specifiable):
+ return v.to_spec()
+
+ if isinstance(v, List):
+ return [_to_spec_helper(e) for e in v]
+
+ if inspect.isfunction(v):
+ if not hasattr(v, "spec_type"):
+ _register(v, inject_spec_type=False)
+ return Spec(type=_get_default_spec_type(v), config=None)
- # a boolean to tell whether the original `__init__` method is called
- _initialized: bool
- # a boolean used by new_getattr to tell whether it is in the `__init__`
method
- # call
- _in_init: bool
+ if inspect.isclass(v):
+ if not hasattr(v, "spec_type"):
+ _register(v, inject_spec_type=False)
+ return Spec(type=_get_default_spec_type(v), config=None)
- @staticmethod
- def _from_spec_helper(v, _run_init):
- if isinstance(v, Spec):
- return Specifiable.from_spec(v, _run_init)
+ return v
- if isinstance(v, List):
- return [Specifiable._from_spec_helper(e, _run_init) for e in v]
- return v
+@runtime_checkable
+class Specifiable(Protocol):
+ """Protocol that a specifiable class needs to implement."""
+ @classmethod
+ def spec_type(cls) -> str:
+ ...
Review Comment:
Generally I think we use `pass` instead of `...` in beam
--
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]
