-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Update mlflow.set_tracking_uri to support pathlib.Path (#5820) #5824
Update mlflow.set_tracking_uri to support pathlib.Path (#5820) #5824
Conversation
@cacharle Thanks for the contribution! The DCO check failed. Please sign off your commits by following the instructions here: https://github.com/mlflow/mlflow/runs/6320105654. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.rst#sign-your-work for more details. |
2defdd4
to
1cf5e21
Compare
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.
@cacharle LGTM once docs errors (https://github.com/mlflow/mlflow/pull/5824/files#r867309500) are addressed. Thanks so much for your contribution!
26135fd
to
649bbef
Compare
Can you also fix the DCO check failure? |
Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.com>
Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.com>
649bbef
to
20f9368
Compare
…erence Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.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!
|
||
|
||
@pytest.mark.parametrize("absolute", [True, False], ids=["absolute", "relative"]) | ||
def test_set_tracking_uri_with_path(tmp_path, monkeypatch, absolute): |
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.
This test fails on windows when absolute
is False
:
D:\a\mlflow\mlflow\mlflow\tracking\_tracking_service\utils.py:75: in set_tracking_uri
uri = uri.resolve().as_uri()
uri = WindowsPath('foo/bar')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = WindowsPath('foo/bar')
def as_uri(self):
"""Return the path as a 'file' URI."""
if not self.is_absolute():
> raise ValueError("relative path can't be expressed as a file URI")
E ValueError: relative path can't be expressed as a file URI
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.
Weird, I call resolve
just before as_uri
so the path should be absolute.
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.
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'm reading https://stackoverflow.com/a/44569249 now.
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 call mkdir
before resolve
?
uri.mkdir(exist_ok=True)
uri = uri.resolve().as_uri()
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 did some test on windows with python3.7 and absolute
seemed to work.
I didn't knew absolute
was deprecated tho
absolute
is more solid now apparently python/cpython#26153
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.
Okay, I checked and mlflow create the directory anyway when we call start_run
.
So mkdir
should be fine.
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.
@cacharle Does this work on windows?
(Path.cwd() / uri).resolve()
Test on Linux
# absolute path
>>> (Path.cwd() / Path("foo").resolve()).resolve()
PosixPath('/home/haru/Desktop/repositories/mlflow/foo')
# relative path
>>> (Path.cwd() / Path("foo")).resolve()
PosixPath('/home/haru/Desktop/repositories/mlflow/foo')
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.
Works on windows aswell
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.
Awesome, let's use this to avoid creating a direcotry. Looks like you already pushed a commit.
Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.com>
Head branch was pushed to by a user without write access
…rectory Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.com>
…h on Windows+Python3.7 Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.com>
@cacharle Thanks for the update! Triggered the CI checks. |
Well, the Or |
…n3.7 .resolve() bug Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.com>
The test failure report indicates
I think we can just do: assert get_tracking_uri() == (tmp_path / path).resolve().as_uri() |
Signed-off-by: Charles Cabergs <charles.cabergs@colruytgroup.com>
LGTM! |
What changes are proposed in this pull request?
Fixes #5820
How is this patch tested?
Test if
set_tracking_uri
convert a path to a URI string.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?
mlflow.set_tracking_uri
now supportspathlib.Path
.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