Remove url casting in local artifact storage path #4890
Conversation
…ocal file location Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
github-actions
bot
added
area/artifacts
Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
…asting for non windows paths Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
…del uri for drive considerations. Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
| @@ -62,7 +62,16 @@ def _download_artifact_from_uri(artifact_uri, output_path=None): | |||
| a local output path will be created. | |||
| """ | |||
| if os.path.exists(artifact_uri): | |||
| artifact_uri = path_to_local_file_uri(artifact_uri) | |||
| if os.name != "nt": | |||
| # If we're dealing with local files, just reference the direct pathing. | |||
There was a problem hiding this comment.
Can we update this inline comment to indicate why we have special treatment for Windows?
mlflow/utils/file_utils.py
Outdated
| if os.name != "nt": | ||
| path = os.path.normpath(path) | ||
| else: | ||
| path = pathname2url(path) |
There was a problem hiding this comment.
Do we still need to make this change now that we have logic in _download_artifacts_from_uri to address the issue? If it's not absolutely necessary, I think we should revert this change.
There was a problem hiding this comment.
This is the key to getting the windows tests to pass. I did a run with this logic disabled and the vast majority of those tests failed on the windows env.
There was a problem hiding this comment.
Why would this be critical to getting windows to pass? The logic for windows remains exactly the same when this change is applied.
| @@ -380,7 +380,8 @@ def test_model_log_without_specified_conda_env_uses_default_env_with_expected_de | |||
|
|
|||
| def test_pyfunc_serve_and_score(prophet_model): | |||
|
|
|||
| artifact_path = "model" | |||
| # Verify that a name with non-url-compliant characters can be handled properly | |||
| artifact_path = "model : prophet" | |||
There was a problem hiding this comment.
Can we add a test case for _download_artifact_from_uri rather than including test coverage in test_prophet_model_export?
mlflow/utils/file_utils.py
Outdated
| if ( | ||
| os.name != "nt" | ||
| ): # Local file paths for non-nt based systems do not need url encoding to resolve paths. | ||
| path = os.path.normpath(path) | ||
| else: # nt-based systems need url-enconding to convert non-supported char types in file names and paths. | ||
| path = pathname2url(path) |
There was a problem hiding this comment.
@BenWilson2 I still don't think we need this logic for windows tests to pass, since the logic is the same for windows users with & without this change applied.
Can we remove this logic?
There was a problem hiding this comment.
I totally forgot about that last minute change I did last night with the additional wrapper in artifact_utils. Reverting this.
Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
| @@ -380,6 +380,7 @@ def test_model_log_without_specified_conda_env_uses_default_env_with_expected_de | |||
|
|
|||
| def test_pyfunc_serve_and_score(prophet_model): | |||
|
|
|||
| # Verify that a name with non-url-compliant characters can be handled properly | |||
There was a problem hiding this comment.
Can we remove this inline comment?
Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
…ting functionality. Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
Signed-off-by: Ben Wilson <benjamin.wilson@databricks.com>
Signed-off-by: Ben Wilson benjamin.wilson@databricks.com
What changes are proposed in this pull request?
Removed the casting to url-encoded pathing in urllib.request.pathname2url() and replaced with os.path.normpath() so that artifacts that have non-url-compliant characters will not get unicode-converted and result in retrieval with pyfunc operators to fail.
Fixes: [4785, 4879]
How is this patch tested?
Unit tests
Release Notes
Is this a user-facing change?
Fixes issues with logging and retrieving pyfunc flavors where the model name value has non-url-compliant characters within its name. (i.e., "sklearn:model", "sklearn model") which would before have been converted (e.g., "sklearn model" -> "sklearn%20model") and would cause a path resolution error on loading.
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