-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add --env-manager
option for mlflow models
commands and deprecate --no-conda
flag
#5567
Conversation
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
--env-manager
option and deprecate --no-conda flag
--env-manager
option and deprecate --no-conda
flag
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
CONDA = "conda" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONDA = "conda" | |
CONDA = "conda" | |
VIRTUALENV = "virtualenv" | |
VIRTUALENV
will be added in #5380
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
--env-manager
option and deprecate --no-conda
flag--env-manager
option for mlflow models
commands and deprecate --no-conda
flag
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
mlflow/utils/cli_args.py
Outdated
@@ -45,12 +49,49 @@ | |||
NO_CONDA = click.option( | |||
"--no-conda", | |||
is_flag=True, | |||
help="If specified, will assume that MLmodel/MLproject is running within " | |||
help="[Deprecated] If specified, will assume that MLmodel/MLproject is running within " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the help message to tell the user to use --env-manager=local
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
mlflow/utils/cli_args.py
Outdated
"environment manager. Valid values are ['local', 'conda']. If unspecified, default to " | ||
"'conda'.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we explain what each of this values means? I.e. local -> use the local environment. conda -> use conda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
mlflow/utils/cli_args.py
Outdated
|
||
ENV_MANAGER = click.option( | ||
"--env-manager", | ||
default=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we default to conda
instead of defaulting to None and resolving later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, suppose old version users he might use command with argument --no-conda
, then when upgrading to new version mlflow command will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently --no-conda
argument is deprecated not removed, so we should keep --no-conda
works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbczumar I chose None
to detect whether or not --env-manager
is specified and distinguish the following cases:
mlflow models ... --no-conda
mlflow models ... --no-conda --env-manager=conda # should fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we could use sys.argv
:
import sys
def _resolve_env_manager(ctx, param, value):
no_conda = ctx.params["no_conda"]
env_manager = EnvManager.from_string(value)
if not no_conda:
return env_manager
is_env_manager_specified = param.opts[0] in sys.argv[1:]
# only --no-conda is specified
if not is_env_manager_specified:
return EnvManager.LOCAL
# Both --no-conda and --env-manager are specified. An exception should be thrown.
raise Exception("...")
ENV_MANAGER = click.option(
"--env-manager",
default="conda",
required=False,
type=click.UNPROCESSED,
callback=_resolve_env_manager,
...
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or can we ignore --env-manager
(and raise a warning) if --no-conda
is supplied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's throw if both are supplied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave this as None
by default; that's fine :). I didn't see the issue before. Makes sense now!
mlflow/utils/cli_args.py
Outdated
) | ||
|
||
|
||
def _validate_env_manager(no_conda, env_manager): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: resolve_env_manager
seems clearer. We're not just performing validation, we're figuring out which internal env manager to use based on the value of no_conda
and env_manager
. Can we add a docstring here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually looking for a better name for this function. resolve_env_manager
sounds much better, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once remaining comments are addressed. Thanks @harupy !
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
… `--no-conda` flag (mlflow#5567) * Introduce --env-manager option and deprecate --no-conda flag Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * add env_manager Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * remove no_conda in PyFuncBackend Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * add from_string Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * add __str__ Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * replace no_conda Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * address comments Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * fix tests Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * set default value for env_manager Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * use --env-manager Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * fix condition Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * use extend Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * address comments Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * use click.BadParameter Signed-off-by: harupy <17039389+harupy@users.noreply.github.com> * fix help text Signed-off-by: harupy <17039389+harupy@users.noreply.github.com>
Signed-off-by: harupy 17039389+harupy@users.noreply.github.com
What changes are proposed in this pull request?
As a stepping stone for pyenv + virtualenv support in MLflow Models & Projects, introduce
--env-manager
option and deprecate--no-conda
.How is this patch tested?
Unit tests
Does this PR change the documentation?
ci/circleci: build_doc
check. If it's successful, proceed to thenext step, otherwise fix it.
Details
on the right to open the job page of CircleCI.Artifacts
tab.docs/build/html/index.html
.Release Notes
Is this a user-facing change?
(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)
What component(s), interfaces, languages, and integrations does this PR affect?
Components
area/artifacts
: Artifact stores and artifact loggingarea/build
: Build and test infrastructure for MLflowarea/docs
: MLflow documentation pagesarea/examples
: Example codearea/model-registry
: Model Registry service, APIs, and the fluent client calls for Model Registryarea/models
: MLmodel format, model serialization/deserialization, flavorsarea/projects
: MLproject format, project running backendsarea/scoring
: MLflow Model server, model deployment tools, Spark UDFsarea/server-infra
: MLflow Tracking server backendarea/tracking
: Tracking Service, tracking client APIs, autologgingInterface
area/uiux
: Front-end, user experience, plotting, JavaScript, JavaScript dev serverarea/docker
: Docker use across MLflow's components, such as MLflow Projects and MLflow Modelsarea/sqlalchemy
: Use of SQLAlchemy in the Tracking Service or Model Registryarea/windows
: Windows supportLanguage
language/r
: R APIs and clientslanguage/java
: Java APIs and clientslanguage/new
: Proposals for new client languagesIntegrations
integrations/azure
: Azure and Azure ML integrationsintegrations/sagemaker
: SageMaker integrationsintegrations/databricks
: Databricks integrationsHow should the PR be classified in the release notes? Choose one:
rn/breaking-change
- The PR will be mentioned in the "Breaking Changes" sectionrn/none
- No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" sectionrn/feature
- A new user-facing feature worth mentioning in the release notesrn/bug-fix
- A user-facing bug fix worth mentioning in the release notesrn/documentation
- A user-facing documentation change worth mentioning in the release notes