robertwb commented on code in PR #26603:
URL: https://github.com/apache/beam/pull/26603#discussion_r1192679459
##########
sdks/python/apache_beam/ml/inference/base.py:
##########
@@ -201,9 +201,11 @@ def get_postprocess_fns(self) -> Iterable[Callable[[Any],
Any]]:
def set_environment_vars(self):
"""Sets environment variables using a dictionary provided via kwargs.
- Keys are the env variable name, and values are the env variable value."""
- for env_variable, env_value in self._env_vars.items():
- os.environ[env_variable] = env_value
+ Keys are the env variable name, and values are the env variable value.
+ Child ModelHandler classes should set _env_vars via kwargs in __init__"""
+ if '_env_vars' in self.__dict__:
Review Comment:
Accessing self.__dict__ directly is a bit hacky, and doesn't always work.
Intead, maybe do
```
for env_variable, env_value in getattr(self, '_env_vars', {}).items():
```
or guard with `hasattr(self, '_env_vars')`
##########
sdks/python/apache_beam/ml/inference/base.py:
##########
@@ -836,7 +838,12 @@ def get_metrics_collector(self, prefix: str = ''):
def setup(self):
self._metrics_collector = self.get_metrics_collector()
- self._model_handler.set_environment_vars()
+ try:
+ self._model_handler.set_environment_vars()
+ except AttributeError:
+ logging.warning(
+ "ModelHander should call super().__init__ or set _env_vars.")
Review Comment:
This is redundant with the guard above, right? Also, arbitrary catching of
`AttributeError` can be dangerous as it masks a lot of other potential problems
(like typos).
Also, if we want this warning, I would suggest calling `super().__init__`
rather than setting `_env_vars` directly, as that is better practice (and will
avoid issues if we add anything else to the base init).
--
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]
