[MLflowSkinny] Base changes for MLflow Skinny Client

[eddiestudies]

What changes are proposed in this pull request?

Adding base changes for MLflow to run with all but numpy, pandas, and sqlparse dependencies removed.

How is this patch tested?

Runs a subset of existing tests to unblock parallel development streams on mlflow skinny client

Release Notes

Is this a user-facing change?

• No. You can skip the rest of this section.
• Yes. Give a description of this change to be included in the release notes for MLflow users.

(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 logging
• area/build: Build and test infrastructure for MLflow
• area/docs: MLflow documentation pages
• area/examples: Example code
• area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
• area/models: MLmodel format, model serialization/deserialization, flavors
• area/projects: MLproject format, project running backends
• area/scoring: Local serving, model deployment tools, spark UDFs
• area/server-infra: MLflow server, JavaScript dev server
• area/tracking: Tracking Service, tracking client APIs, autologging

Interface

• area/uiux: Front-end, user experience, JavaScript, plotting
• area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
• area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
• area/windows: Windows support

Language

• language/r: R APIs and clients
• language/java: Java APIs and clients
• language/new: Proposals for new client languages

Integrations

• integrations/azure: Azure and Azure ML integrations
• integrations/sagemaker: SageMaker integrations
• integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

• rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
• rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
• rn/feature - A new user-facing feature worth mentioning in the release notes
• rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
• rn/documentation - A user-facing documentation change worth mentioning in the release notes

[dbczumar]

@harupy @smurching Didn't this come up in the context of another ticket? Can you comment on how to best resolve it?

[smurching]

It did yeah - we were confused why e.g. gorilla was still being installed in our test environment despite the fact that we'd removed it as a dependency from setup.py, e.g. in https://github.com/mlflow/mlflow/pull/3666/checks?check_run_id=1382567094#step:3:308. It turns out in our dependency installation scripts for Github actions, we install this test plugin and then install MLflow from source - because we install the test plugin first, we end up installing mlflow from pypi as a transitive dependency of the plugin, and then overwriting that installation with MLflow from source.

The change here avoids that problem by removing mlflow as a dependency from the test plugin. This seems ok as we ought to only use the test plugin in our CI - in practice though, we reference the test plugin in our docs on writing plugins.

To get around this we could either:
a) Define another example plugin to reference in our docs & add tests verifying that it works (seems like more work)
b) See if we can do something like install_requires=[os.path.join(".", os.pardir, os.pardir, os.pardir)] to depend on MLflow installed from source (reduces the value of the test plugin as an example)
c) Just update our install-common-deps.sh CI script to install MLflow from source before we install other Python dependencies, including the test plugin

[smurching]

IMO option a) is maybe the best long-term approach, but I'd be fine with option c) too in practice as a quick fix for this issue; the risk is that one of our test requirements has a strict version dependency on mlflow and ends up overwriting our source installation of mlflow, but this seems unlikely / we could potentially test for this

[dbczumar]

As discussed offline with @smurching , I think the concern here is that depending on mlflow (even a source-installed copy) will bring in dependencies that aren't included by the skinny client, degrading the utility of our skinny client tests.

As a result, I think it's best for us to adopt option (a) here. @eedeleon Is this something you can do as part of this PR? If not, I can pitch in.

[eddiestudies]

I think we could implement a like b.

requirements = [] if MLFLOW_TEST_ENV in os.environ else ["mlflow"]

[eddiestudies]

We can add a test that verifies MLFLOW_TEST_ENV(we can use any env var) is set as well

[dbczumar]

That os.environ code will show up in our example reference too, though, which we probably don't want.

[dbczumar]

We can add a test that verifies MLFLOW_TEST_ENV(we can use any env var) is set as well

Agreed!

[dbczumar]

@eedeleon Sorry for the delay here; I left a few comments. This is close!

[dbczumar]

Since we're installing sqlalchemy / alembic / sqlparse for integration testing of tracking APIs, can we run test_tracking.py in addition to test_client.py? test_client.py relies on mocked backend stores & doesn't seem to explicitly require sqlalchemy functionality.

In a future PR, we should more explicitly restructure our test coverage to separate unit & integration tests for clarity, as per our offline discussion. E.g.,:

- Tracking
    - Client
        - Unit (Tests that don’t depend on a backend)
        - Integration (Tests for client interaction against a backend)
    - Server
        - Unit (Backend / sever tests that don’t depend on a client)
        - Integration (Backend / server tests that depend on a client - e.g., `test_rest_tracking.py`)

Leaving this here as a reminder for us to revisit test structure prior to a skinny client release. More than happy to formalize this as a more complete refactoring plan!

[dbczumar]

The skinny client will eventually need to support databricks_artifact_repo for compatibility with Databricks. Leaving this here as a note for myself to remove the dependency on azure.storage.blob.BlobClient for this store. We've done this already for a different compatibility purpose on Databricks, so it should be relatively quick to apply this change.

[dbczumar]

As discussed offline with @smurching , I think the concern here is that depending on mlflow (even a source-installed copy) will bring in dependencies that aren't included by the skinny client, degrading the utility of our skinny client tests.

As a result, I think it's best for us to adopt option (a) here. @eedeleon Is this something you can do as part of this PR? If not, I can pitch in.

[dbczumar]

Is the concern here that we might deploy an MLflow model to AzureML and install an older version of MLflow when creating the serving container Docker image? The deployment procedure seems to ensure that we're pulling in the latest MLflow version (

mlflow/mlflow/azureml/__init__.py

Lines 554 to 561 in aaed804

	if mlflow_path is not None:
	mlflow_install_cmd = "RUN pip install -e {mlflow_path}".format(
	mlflow_path=_get_container_path(mlflow_path)
	)
	elif not mlflow_version.endswith("dev"):
	mlflow_install_cmd = "RUN pip install mlflow=={mlflow_version}".format(
	mlflow_version=mlflow_version
	)

), so I think it's okay to remove the try/except here.

Let me know if I'm missing something here.

[dbczumar]

Nit: Can we move this down to line 78-ish (right above setup()). The var naming + comments look awesome - thanks so much for the investment there!

[dbczumar]

Sorry, I think I got this wrong. This run_local doesn't seem like it actually starts a flask server. Can we revert this move?

[dbczumar]

I don't think we should embed the UI JS, model container server files, or alembic files with the skinny client. Can we condition adding these files on _is_mlflow_skinny and omit them if skinny is True?

[dbczumar]

Hmm...looks like these files are included independently of package_data. I think find_packages picks them up. Let's follow up here in a future PR.

[dbczumar]

@eedeleon This is really close. I left a few more comments.

The most significant thing I noticed while trying this out is that attempting to run the MLflow CLI fails with an alembic import error:

import mlflow.cli
ModuleNotFoundError: No module named 'alembic'

Ideally, we should provide test coverage for the MLflow CLI. I noticed that test_cli aggregates test cases for mlflow server, mlflow ui, mlflow gc, and mlflow run. Perhaps we could split this file up: 1. add the mlflow server / mlflow ui /mlflow gc tests to a new test_cli suite in the tests/server subdirectory & 2. add the mlflow run tests to test_projects_cli.

Otherwise, I've confirmed that the following workflows are supported by the skinny client on Databricks:

• Tracking operations (logging / loading / searching params, metrics, tags + logging / loading artifacts on AWS - Azure requires removal of the Azure Storage Blob dependency as a follow-up PR)

• Model registration, search, artifact loading, transitions

• Execution of GitHub projects within a Databricks notebook & against a Databricks workspace remotely

• After installing scikit-learn, sklearn autologging + model log / load operations succeed

[dbczumar]

@eedeleon LGTM once a few small comments have been addressed! Thanks so much for this contribution!

Here's a TODO list of follow-up tasks I've put together; ones necessary prior to the initial skinny client release are tagged [Required for 1.14]:

• [Required for 1.14] Figure out how to distribute the client. E.g., add to MLflow release CLI scaffolding maintained by Databricks

• [Required for 1.14] Document the skinny client on mlflow.org / in the GitHub repository (what does the skinny client do and what does it not do)

• [Required for 1.14] Drop azure-storage-blob requirement for artifact logging on Azure Databricks

• Drop pandas and numpy requirements

• Test restructure: https://github.com/mlflow/mlflow/pull/3687/files#r528014570

[dbczumar]

@eedeleon Looks like azureml and sagemaker CLI command imports both succeed. mlflow.db succeeds if we move

mlflow/mlflow/db.py

Line 3 in da9a7d1

import mlflow.store.db.utils

down here

mlflow/mlflow/db.py

Line 16 in da9a7d1

def upgrade(url):

.

I think it might be easier to make the fix for db.py right now, rather than adding the warning log. Otherwise, the user sees a warning message every time they use the mlflow CLI, which looks like an error that the user needs to address rather than an expected limitation of the skinny client:

mlflow --help

2020/12/17 11:56:40 WARNING mlflow.cli: Failed to import mlflow.db with No module named 'alembic'.
Usage: mlflow [OPTIONS] COMMAND [ARGS]...

Options:
  --version  Show the version and exit.
  --help     Show this message and exit.

Commands:
  artifacts    Upload, list, and download artifacts from an MLflow artifact...
  azureml      Serve models on Azure ML.
  deployments  Deploy MLflow models to custom targets.
  experiments  Manage experiments.
  gc           Permanently delete runs in the `deleted` lifecycle stage.
  models       Deploy MLflow models locally.
  run          Run an MLflow project from the given URI.
  runs         Manage runs.
  sagemaker    Serve models on SageMaker.
  server       Run the MLflow tracking server.
  ui           Launch the MLflow tracking UI for local viewing of run...

[dbczumar]

@eedeleon Is this necessary? sqlalchemy, alembic, and sqlparse shouldn't be present when we install the skinny client. Explicitly uninstalling them seems to allow for them to be present initially, indicating that our tests are permitting an erroneous condition.

[eddiestudies]

sqlalchemy was recently added to small-requirements.txt.

I can create a skinny-requirements.txt and use that to avoid further issues but it seemed like overkill.

Should I go with the skinny-requirements.txt or only uninstall sqlalchemy?

[dbczumar]

@eedeleon Can we move these into tests/projects/cli.py?

At a glance, it doesn't really seem like any of these test cases should be causing the permissions-related issue described in #3888.

[dbczumar]

@eedeleon How do we know that installing MLflow Skinny didn't pull in sqlalchemy independently of the Python small dependencies? We could change the ordering here so that the MLflow client is installed after the small dependencies & removal of sqlalchemy, but I'd rather create a skinny-requirements file instead of relying on subtle ordering properties / environment-variable-based branching logic in install-common-deps.

[eddiestudies]

I made the change. There is some overlap between small-requirements and skinny requirements, but I agree it is worth it since this approach is definitely more explicit and less likely to leak bugs.