Added an environment variable flag to recreate the AMLS Environment #230
Conversation
ml_service/util/env_variables.py
Outdated
| @@ -46,6 +46,8 @@ def __init__(self): | |||
| self._allow_run_cancel = os.environ.get( | |||
| "ALLOW_RUN_CANCEL", "true") | |||
| self._aml_env_name = os.environ.get("AML_ENV_NAME") | |||
| self._rebuild_env = os.environ.get("AML_REBUILD_ENVIRONMENT", "true").lower().strip() | |||
| @@ -31,7 +31,7 @@ def main(): | |||
| # Make sure to include `r-essentials' | |||
| # in diabetes_regression/conda_dependencies.yml | |||
| environment = get_environment( | |||
| aml_workspace, e.aml_env_name, create_new=False) # NOQA: E501 | |||
| aml_workspace, e.aml_env_name, create_new=e.rebuild_env == "true") # NOQA: E501 | |||
There was a problem hiding this comment.
Please remove the == "true" so it can just be whatever the actual value is
There was a problem hiding this comment.
Hmm I don't understand: the environment variables are not well-typed (i.e. they are strings). In that case the flag would evaluate to true all the time (as a non-empty string). Just like here
There was a problem hiding this comment.
I'd have to check with an actual run or dig into the SDK, but I'm betting the create_new=False will be ok with False as a string (it's not checking it as a boolean).
There was a problem hiding this comment.
so the variable should be boolean, (see here) and Python will convert any type to boolean, but with unexpected results :)
so the check for the "true" string is vital here for this to work
There was a problem hiding this comment.
An alternative solution could be to use a library like https://pypi.org/project/environs/ to type the environment variables properly.
There was a problem hiding this comment.
Environs looks cool, but I don't really want to introduce a new dependency right now. @sudivate thoughts?
There was a problem hiding this comment.
Looks good to me, for the scope of this PR lets typecast the string to bool in env_variables.py
| @@ -33,8 +33,7 @@ def main(): | |||
|
|
|||
| # Create a reusable Azure ML environment | |||
| environment = get_environment( | |||
| aml_workspace, e.aml_env_name, create_new=False) # NOQA: E501 | |||
|
|
|||
| aml_workspace, e.aml_env_name, create_new=e.rebuild_env == "true") # | |||
There was a problem hiding this comment.
Please remove the == "true" so it can just be whatever the actual value is
There was a problem hiding this comment.
Hmm I don't understand: the environment variables are not well-typed (i.e. they are strings). In that case the flag would evaluate to true all the time (as a non-empty string). Just like here
(same as above)
There was a problem hiding this comment.
That's right. It's confusing in Python. I would update env_variables.py to return a boolean (not string) value. E.g. return self._rebuild_env == "true"
…Python into cm/rebuild-environment-flag
Fixes #229 by allowing the AML Environment to be recreated instead of reused.