Added an environment variable flag to recreate the AMLS Environment#230
Added an environment variable flag to recreate the AMLS Environment#230dtzar merged 8 commits intomicrosoft:masterfrom
Conversation
ml_service/util/env_variables.py
Outdated
| 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() |
There was a problem hiding this comment.
Please set to false by default
| # 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
| 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
4 participants
Fixes #229 by allowing the AML Environment to be recreated instead of reused.