Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e63182b
Add protos and compilation
dbczumar May 12, 2020
cb69e26
Adding databricks_artifact_repo to store/artifact
arjundc-db May 19, 2020
3af5d3b
Addressing comments
arjundc-db May 19, 2020
38630d3
Addressing comments and fixing _download_file
arjundc-db May 20, 2020
1cb98e9
Code Clean-up and lint
arjundc-db May 20, 2020
870c0aa
Adding multi-part upload logic and unit tests
arjundc-db May 22, 2020
10cf458
Addressing comments
arjundc-db May 26, 2020
ecdce6a
Addressing comments, making azure download more memory efficent and o…
arjundc-db May 27, 2020
116c5d0
Small fix
arjundc-db May 27, 2020
11be341
Fixing list_artifacts
arjundc-db May 28, 2020
0609a5b
Addressing final comments.
arjundc-db Jun 1, 2020
3f1327d
Making extract_run_id static
arjundc-db Jun 1, 2020
98035b4
Adding AWS support
arjundc-db Jun 2, 2020
0b1af46
Addressing comments
arjundc-db Jun 4, 2020
3d923cb
Fix - needs docs and tests
dbczumar Jun 8, 2020
d3fab4a
Comment and simplification
dbczumar Jun 8, 2020
368973d
Special case for empty file upload to AWS
arjundc-db Jun 8, 2020
b67e4dc
Merge pull request #4 from dbczumar/databricks-repo-fix
arjundc-db Jun 8, 2020
5e87cf7
Merge branch 'databricks-artifact-repo' of https://github.com/arjundc…
arjundc-db Jun 8, 2020
72427a3
Clean up and added tests for relative path
arjundc-db Jun 8, 2020
f82c607
Page
dbczumar Jun 9, 2020
30e3885
Fix
dbczumar Jun 9, 2020
a03e099
Added relative path test cases
arjundc-db Jun 10, 2020
50a5e28
Merge pull request #5 from dbczumar/databricks-repo-pagination
arjundc-db Jun 10, 2020
8c75590
Merge branch 'databricks-artifact-repo' of https://github.com/arjundc…
arjundc-db Jun 10, 2020
6d36297
Added test for list_artifacts pagination
arjundc-db Jun 10, 2020
d84fa21
Merge branch 'master' into databricks-artifact-repo
dbczumar Jun 15, 2020
ec85f06
Fixing travis failures
arjundc-db Jun 16, 2020
70ca1d7
Merge branch 'databricks-artifact-repo' of https://github.com/arjundc…
arjundc-db Jun 16, 2020
c6585d6
Fixes
arjundc-db Jun 16, 2020
51ca61f
More fixes
arjundc-db Jun 16, 2020
d1b9dfd
More fixes
arjundc-db Jun 16, 2020
72ecab5
Clean-up
arjundc-db Jun 16, 2020
dfc7f60
Clean-up
arjundc-db Jun 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions generate-protos.sh
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,18 @@ protoc -I="$PROTOS" \
"$PROTOS"/databricks.proto \
"$PROTOS"/service.proto \
"$PROTOS"/model_registry.proto \
"$PROTOS"/databricks_artifacts.proto \
"$PROTOS"/scalapb/scalapb.proto

OLD_SCALAPB="from scalapb import scalapb_pb2 as scalapb_dot_scalapb__pb2"
NEW_SCALAPB="from .scalapb import scalapb_pb2 as scalapb_dot_scalapb__pb2"
sed -i'.old' -e "s/$OLD_SCALAPB/$NEW_SCALAPB/g" "$PROTOS/databricks_pb2.py" "$PROTOS/service_pb2.py" "$PROTOS/model_registry_pb2.py"
sed -i'.old' -e "s/$OLD_SCALAPB/$NEW_SCALAPB/g" "$PROTOS/databricks_pb2.py" "$PROTOS/service_pb2.py" "$PROTOS/model_registry_pb2.py" "$PROTOS/databricks_artifacts_pb2.py"

OLD_DATABRICKS="import databricks_pb2 as databricks__pb2"
NEW_DATABRICKS="from . import databricks_pb2 as databricks__pb2"
sed -i'.old' -e "s/$OLD_DATABRICKS/$NEW_DATABRICKS/g" "$PROTOS/service_pb2.py" "$PROTOS/model_registry_pb2.py"
sed -i'.old' -e "s/$OLD_DATABRICKS/$NEW_DATABRICKS/g" "$PROTOS/service_pb2.py" "$PROTOS/model_registry_pb2.py" "$PROTOS/databricks_artifacts_pb2.py"

rm "$PROTOS/databricks_pb2.py.old"
rm "$PROTOS/service_pb2.py.old"
rm "$PROTOS/model_registry_pb2.py.old"
rm "$PROTOS/databricks_artifacts__pb2.py.old"
113 changes: 113 additions & 0 deletions mlflow/protos/databricks_artifacts.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
syntax = "proto2";

package mlflow;

import "scalapb/scalapb.proto";
import "databricks.proto";

option java_package = "com.databricks.api.proto.mlflow";
option java_generate_equals_and_hash = true;
option py_generic_services = true;
option (scalapb.options) = {
flat_package: true,
};

service DatabricksMlflowArtifactsService {

// Fetch credentials to read from the specified MLflow artifact location
//
// Note: Even if no artifacts exist at the specified artifact location, this API will
// still provide read credentials as long as the format of the location is valid.
// Callers must subsequently check for the existence of the artifacts using the appropriate
// cloud storage APIs (as determined by the `ArtifactCredentialType` property of the response)
rpc getCredentialsForRead (GetCredentialsForRead) returns (GetCredentialsForRead.Response) {
option (rpc) = {
endpoints: [{
method: "GET",
path: "/mlflow/artifacts/credentials-for-read"
since { major: 2, minor: 0 },
}],
visibility: PUBLIC_UNDOCUMENTED,
};
}

// Fetch credentials to write to the specified MLflow artifact location
rpc getCredentialsForWrite (GetCredentialsForWrite) returns (GetCredentialsForWrite.Response) {
option (rpc) = {
endpoints: [{
method: "GET",
path: "/mlflow/artifacts/credentials-for-write"
since { major: 2, minor: 0 },
}],
visibility: PUBLIC_UNDOCUMENTED,
};
}
}

// The type of a given artifact access credential
enum ArtifactCredentialType {

// The credential is an Azure Shared Access Signature URI. For more information, see
// https://docs.microsoft.com/en-us/azure/storage/common/storage-sas-overview
AZURE_SAS_URI = 1;

// The credential is an AWS Presigned URL. For more information, see
// https://docs.aws.amazon.com/AmazonS3/latest/dev/ShareObjectPreSignedURL.html
AWS_PRESIGNED_URL = 2;

}

message ArtifactCredentialInfo {

// The ID of the MLflow Run containing the artifact that can be accessed
// with the credential
optional string run_id = 1;

// The path, relative to the Run's artifact root location, of the artifact
// that can be accessed with the credential
optional string path = 2;

// The signed URI credential that provides access to the artifact
optional string signed_uri = 3;

// The type of the signed credential URI (e.g., an AWS presigned URL
// or an Azure Shared Access Signature URI)
optional ArtifactCredentialType type = 4;

}

message GetCredentialsForRead {
option (scalapb.message).extends = "com.databricks.rpc.RPC[$this.Response]";

// The ID of the MLflow Run for which to fetch artifact read credentials
optional string run_id = 1;

// The artifact path, relative to the Run's artifact root location, for which to
// fetch artifact read credentials
optional string path = 2;

message Response {

// Credentials for reading from the specified artifact location
optional ArtifactCredentialInfo credentials = 1;

}
}

message GetCredentialsForWrite {
option (scalapb.message).extends = "com.databricks.rpc.RPC[$this.Response]";

// The ID of the MLflow Run for which to fetch artifact write credentials
optional string run_id = 1;

// The artifact path, relative to the Run's artifact root location, for which to
// fetch artifact write credentials
optional string path = 2;

message Response {

// Credentials for writing to the specified artifacts location
optional ArtifactCredentialInfo credentials = 1;

}
}
94 changes: 94 additions & 0 deletions mlflow/store/artifact/databricks_artifact_repo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
from azure.storage.blob import BlobClient
Comment thread
dbczumar marked this conversation as resolved.

import os
from mlflow.exceptions import MlflowException
from mlflow.store.artifact.artifact_repo import ArtifactRepository
from mlflow.utils.string_utils import strip_suffix
from mlflow.utils.file_utils import relative_path_to_artifact_path
from mlflow.utils.rest_utils import call_endpoint, extract_api_info_for_service
from mlflow.protos.databricks_artifacts_pb2 import DatabricksMlflowArtifactsService
from mlflow.protos.databricks_artifacts_pb2 import GetCredentialsForWrite, GetCredentialsForRead
from mlflow.utils.databricks_utils import get_databricks_host_creds
from mlflow.protos.service_pb2 import MlflowService, ListArtifacts

_PATH_PREFIX = "/api/2.0"


class DatabricksArtifactRepository(ArtifactRepository):
"""
SOMETHING : TYPING TILL IT WORKS LOL
"""

def __init__(self, artifact_uri):
super(DatabricksArtifactRepository, self).__init__(artifact_uri)

def _extract_run_id(self, artifact_uri):
return artifact_uri.lstrip('/').split('/')[4]

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arjundc-db Awesome that we're extracting the run ID from the URI! A quick suggestion here - can we: 1. use the urlparse library to fetch the path component of the URI and 2. normalize the path before performing the splitting? E.g.,

import posixpath
from urllib.parse import urlparse
parsed_uri = urlparse(artifact_uri)
parsed_path = posixpath.normpath(parsed_uri.path)
return parsed_path.lstrip("/").split("/")[4]

The reasoning for #1 here is that DBFS URIs support empty hostnames; e.g., dbfs:///databricks/mlflow-tracking/EXP_ID/RUN_ID is valid (note the 3 leading slashes). I confirmed this by running %fs ls dbfs:///databricks in a Databricks notebook. The current solution yields the incorrect result when applied directly to a URI of this form:

In [1]: url = "dbfs:///databricks/mlflow-tracking/EXP_ID/RUN_ID/artifacts"
In [2]: url.lstrip('/').split('/')[4]
Out[2]: 'mlflow-tracking' # Should be 'RUN_ID'

Similarly, I've confirmed that dbfs://databricks/ (two leading slashes) and dbfs:databricks/... (no leading slashes) are invalid use cases; Databricks rejects URIs of the first form because they contain non-empty hostnames, which are unsupported. Databricks rejects URIs of the second form because they contain relative paths. Databricks requires URIs to specify absolute DBFS paths. This means that we don't have to worry about relative paths, which may not contain a leading slash.

The reasoning for #2 here is that URI paths may contain redundant slashes. For example, dbfs:/databricks/mlflow-tracking////EXP_ID/RUN_ID is a valid URI that is semantically equivalent to dbfs:/databricks/mlflow-tracking/EXP_ID/RUN_ID. Normalizing the path will remove redundant slashes. Without normalizing the path, we currently get an incorrect result:

In [1]: url = "dbfs:/databricks////mlflow-tracking/EXP_ID/RUN_ID/"
In [2]: url.lstrip('/').split('/')[4]
Out [2]: ''

For more information about the structure of URIs, https://en.wikipedia.org/wiki/Uniform_Resource_Identifier is an awesome reference. Let me know if you have any questions here.

These kinds of URI edge cases would be great to handle via a unit test. Let's make sure we plan to add a unit test for this case before merging this functionality!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we correct the behavior here, can we also add an inline comment that clearly documents the assumptions we're making about the URI structure? E.g., URIs are assumed to be semantically equivalent to dbfs:/databricks/mlflow-tracking/EXP_ID/RUN_ID/artifacts/... ? This will help future readers of the code interpret the behavior.

@arjundc-db arjundc-db May 19, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, this was pretty cool to read about, thanks!


def _call_endpoint(self, service, api, json_body):
_METHOD_TO_INFO = extract_api_info_for_service(service, _PATH_PREFIX)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extract_api_info_for_service method is a bit expensive (https://github.com/mlflow/mlflow/blob/c4e0cbdf5c92b3b81f7080fff4a974114605d2db/mlflow/utils/rest_utils.py#L115). Can we instead define a module-level _SERVICE_AND_METHOD_TO_INFO dictionary as:

_SERVICE_AND_METHOD_TO_INFO = {
    service: extract_api_info_for_service(service, _PATH_PREFIX)
    for service in [MlflowService, DatabricksMlflowArtifactsService]
}

We can then fetch the info as _SERVICE_AND_METHOD_TO_INFO[service][api], without performing proto extraction operations each time this method is called.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

endpoint, method = _METHOD_TO_INFO[api]
response_proto = api.Response()
return call_endpoint(get_databricks_host_creds(), endpoint, method, json_body, response_proto)

def _create_json_body(self, run_id, path=None):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arjundc-db I think this is unused. Can we remove it?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks for the catch.

path = path or '.'

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
path = path or '.'
path = path or ""

"" is more idiomatic for representing empty than "."

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return {
"run_id": run_id,
"path": path
}

def _get_azure_write_credentials(self, run_id, path=None):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method calls an MLflow Artifacts Service endpoint that is not directly bound to Azure; it may also return AWS credentials. The type of credential is specified by the type field of the ArtifactCredentialInfo response. Accordingly, I think we should call this _get_write_credentials.

Callers of this method should check the type field of the response and then perform the corresponding cloud operation (either AWS or Azure). The current task is to implement Azure uploads / downloads, so it's fine to leave the AWS case unimplemented (e.g., simply raise a "Not Implemented" exception in the code). It would still be great to structure the upload / download logic so that it accounts for the fact that credentials may be provided for different cloud services. Let me know if this makes sense!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return self._call_endpoint(DatabricksMlflowArtifactsService, GetCredentialsForWrite,
self._create_json_body(run_id, path))

def _get_azure_read_credentials(self, run_id, path=None):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method calls an MLflow Artifacts Service endpoint that is not directly bound to Azure; it may also return AWS credentials. The type of credential is specified by the type field of the ArtifactCredentialInfo response. Accordingly, I think we should call this _get_read_credentials.

Callers of this method should check the type field of the response and then perform the corresponding cloud operation (either AWS or Azure). The current task is to implement Azure uploads / downloads, so it's fine to leave the AWS case unimplemented (e.g., simply raise a "Not Implemented" exception in the code). It would still be great to structure the upload / download logic so that it accounts for the fact that credentials may be provided for different cloud services. Let me know if this makes sense!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return self._call_endpoint(DatabricksMlflowArtifactsService, GetCredentialsForRead,
self._create_json_body(run_id, path))

def _upload_file(self, local_file, artifact_path):
run_id = self._extract_run_id(self.artifact_uri)
write_credentials = self._get_azure_write_credentials(run_id, artifact_path)
signed_write_uri = write_credentials.credentials.signed_uri
service = BlobClient.from_blob_url(blob_url=signed_write_uri, credential=None)
try:
with open(local_file, "rb") as data:
service.upload_blob(data, overwrite=True)
except Exception as err:
raise MlflowException(err)

def log_artifact(self, local_file, artifact_path=None):
self._upload_file(local_file, artifact_path)

def log_artifacts(self, local_dir, artifact_path=None):
artifact_path = artifact_path or ''
basename = os.path.basename(strip_suffix(local_dir, '/'))
for (dirpath, _, filenames) in os.walk(local_dir):
artifact_subdir = basename
if dirpath != local_dir:
rel_path = os.path.relpath(dirpath, local_dir)
rel_path = relative_path_to_artifact_path(rel_path)
artifact_subdir = os.path.join(artifact_subdir, rel_path)
for name in filenames:
local_file = os.path.join(dirpath, name)
artifact_location = os.path.join(artifact_path, artifact_subdir)
self._upload_file(local_file, artifact_location)

def list_artifacts(self, path=None):
run_id = self._extract_run_id(self.artifact_uri)
return self._call_endpoint(MlflowService, ListArtifacts, self._create_json_body(run_id, path))

def _download_file(self, remote_file_path, local_path):
run_id = self._extract_run_id(self.artifact_uri)
read_credentials = self._get_azure_read_credentials(run_id, remote_file_path)
signed_read_uri = read_credentials.credentials.signed_uri
service = BlobClient.from_blob_url(blob_url=signed_read_uri, credential=None)
try:
with open(local_path, "wb") as output_file:
blob = service.download_blob()
output_file.write(blob.readall())
except Exception as err:
raise MlflowException(err)

def delete_artifacts(self, artifact_path=None):
raise MlflowException('Not implemented yet')
6 changes: 5 additions & 1 deletion mlflow/store/artifact/dbfs_artifact_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@
from mlflow.exceptions import MlflowException
from mlflow.store.tracking.rest_store import RestStore
from mlflow.store.artifact.artifact_repo import ArtifactRepository
from mlflow.store.artifact.databricks_artifact_repo import DatabricksArtifactRepository
from mlflow.store.artifact.local_artifact_repo import LocalArtifactRepository
from mlflow.tracking._tracking_service import utils
from mlflow.utils.file_utils import relative_path_to_artifact_path
from mlflow.utils.rest_utils import http_request, http_request_safe, RESOURCE_DOES_NOT_EXIST
from mlflow.utils.string_utils import strip_prefix
import mlflow.utils.databricks_utils
from mlflow.utils.uri import is_artifact_acled_uri

LIST_API_ENDPOINT = '/api/2.0/dbfs/list'
GET_STATUS_ENDPOINT = '/api/2.0/dbfs/get-status'
Expand Down Expand Up @@ -163,7 +165,9 @@ def dbfs_artifact_repo_factory(artifact_uri):
:return: Subclass of ArtifactRepository capable of storing artifacts on DBFS.
"""
cleaned_artifact_uri = artifact_uri.rstrip('/')
if mlflow.utils.databricks_utils.is_dbfs_fuse_available() \
if is_artifact_acled_uri(artifact_uri):
return DatabricksArtifactRepository(artifact_uri)
elif mlflow.utils.databricks_utils.is_dbfs_fuse_available() \
and os.environ.get(USE_FUSE_ENV_VAR, "").lower() != "false" \
and not artifact_uri.startswith("dbfs:/databricks/mlflow-registry"):
# If the DBFS FUSE mount is available, write artifacts directly to /dbfs/... using
Expand Down
1 change: 0 additions & 1 deletion mlflow/utils/databricks_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from mlflow.utils.rest_utils import MlflowHostCreds
from databricks_cli.configure import provider


_logger = logging.getLogger(__name__)


Expand Down
6 changes: 5 additions & 1 deletion mlflow/utils/uri.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

_INVALID_DB_URI_MSG = "Please refer to https://mlflow.org/docs/latest/tracking.html#storage for " \
"format specifications."

_ACLED_ARTIFACT_URI = "dbfs:/databricks/mlflow-tracking/"

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can we move this into is_artifact_acled_uri()? It doesn't seem to be required by more than one function in this module.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


def is_local_uri(uri):
"""Returns true if this is a local file path (/foo or file:/foo)."""
Expand Down Expand Up @@ -129,3 +129,7 @@ def _join_posixpaths_and_append_absolute_suffixes(prefix_path, suffix_path):
# joined path
suffix_path = suffix_path.lstrip(posixpath.sep)
return posixpath.join(prefix_path, suffix_path)


def is_artifact_acled_uri(artifact_uri):

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same vein as the comment above about semantically equivalent URIs (e.g., dbfs:///databricks/mlflow-tracking////EXP_ID/RUN_ID/...), this check will fail. Can we do the following instead?:

  1. Parse the URI
  2. Extract the scheme and verify that it's dbfs
  3. Extract the path, normalize it via posixpath.normpath()
  4. Verify that the normalized path has the expected prefix

It would also be great to unit test this method against various semantically equivalent URIs.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, however, I do not think step 2 is necessary since https://livegrep.dev.databricks.com/view/mlflow/mlflow/mlflow/store/artifact/artifact_repository_registry.py#L63 will only call dbfs_artifact_repo_factory when the scheme is dbfs.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arjundc-db I think we're relying on an assumption about exactly where this function is going to be called. Future callers of this function are not guaranteed to perform dbfs scheme validation beforehand. Accordingly, we have a few options:

  1. Perform the scheme validation in this method
  2. Do not perform the scheme validation in this method and clearly document (via the method docstring) that this method assumes it is being passed a dbfs:/ URI and will not validate the scheme
  3. If we don't foresee this method being called elsewhere, we can define it within the dbfs_artifact_repo_factory() method as a subroutine. This way, we can safely assume that we're already working with a dbfs URI (as documented by the dbfs_artifact_repo_factory docstring).

I'd vote for either option 1 or option 3!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that makes sense. Using option 3.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we rename this to is_databricks_acled_artifacts_uri? This helps to clarify that we're checking for a piece of Databricks-specific functionality.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return artifact_uri.startswith(_ACLED_ARTIFACT_URI.lstrip('/'))