birdie icon indicating copy to clipboard operation
birdie copied to clipboard

chore: add nvm support

Open escapedcat opened this issue 3 years ago • 1 comments

Support for nvm helps people like me to on their first install.
It will automatically use node v12 if you are using nvm.
Everything works like before if you don't use nvm. It's fine.

Minor format changes on the README due to prettier-plugin. I can revert these if wanted.

escapedcat avatar Jan 12 '22 06:01 escapedcat

@TomSteenbergen Thanks for reporting this. Your point makes sense. Let me check why we chose to exclude setputools or didn't add setuptools as an mlflow dependency.

harupy avatar Mar 07 '24 00:03 harupy

Thanks @harupy! Let me know if I can help by filing a PR.

TomSteenbergen avatar Mar 07 '24 08:03 TomSteenbergen

@mlflow/mlflow-team Please assign a maintainer and start triaging this issue.

github-actions[bot] avatar Mar 13 '24 00:03 github-actions[bot]

We've been hitting this for a while now on python 3.12. Gotten around it by adding setuptools as an explicit install dependency to projects using mlflow.

This is the only place it's used in the code base:

https://github.com/mlflow/mlflow/blob/79db4bc0ba7d147f168d5194ec0f91a5f7a8e442/mlflow/utils/requirements_utils.py#L165

I tried replacing pkg_resources with calls to importlib_metadata but that was causing tests to fail. Still haven't gotten to the bottom of it but pkg_resources was known to be buggy (that's part of why its deprecated).

I tried the below patch out but think someone more familiar with how this code is being used may need to take a look in to the test failure after applying it. pytest gets removed from the list of dependencies in tests/utils/test_requirements_utils.py::test_infer_requirements_excludes_mlflow.

git diff upstream/master
diff --git a/mlflow/utils/requirements_utils.py b/mlflow/utils/requirements_utils.py
index 57cb4ab92..3ec8f558a 100644
--- a/mlflow/utils/requirements_utils.py
+++ b/mlflow/utils/requirements_utils.py
@@ -17,7 +17,6 @@ from threading import Timer
 from typing import List, NamedTuple, Optional

 import importlib_metadata
-import pkg_resources  # noqa: TID251
 from packaging.requirements import Requirement
 from packaging.version import InvalidVersion, Version

@@ -162,10 +161,17 @@ def _normalize_package_name(pkg_name):

 def _get_requires(pkg_name):
     norm_pkg_name = _normalize_package_name(pkg_name)
-    if package := pkg_resources.working_set.by_key.get(norm_pkg_name):
-        for req in package.requires():
-            yield _normalize_package_name(req.name)
+    # Try to look up the distribution, if it is not installed, don't require it.
+    # Ex. Package is part of an uninstalled extra or installed with --no-deps.
+    requires = None
+    try:
+        requires = importlib_metadata.requires(norm_pkg_name)
+    except importlib_metadata.PackageNotFoundError:
+        _logger.debug("Package %s not found in environment", norm_pkg_name)

+    if requires is not None:
+        for req in requires:
+            yield _normalize_package_name(Requirement(req).name)

importlib.metadata.distributions() has been pretty much a drop in replacement for the working set in other projects. Maybe the code could be a bit simplified down if we're just looking for everything that's installed?


def _get_pip_deps() -> list[str]:
    excluded = {
        "pip",
        "setuptools",
    }
    packages = [
        f"{dist.metadata['Name']}=={dist.version}"
        for dist in importlib.metadata.distributions()
        if dist.metadata["Name"] not in excluded
    ]
    return packages

jmahlik avatar Mar 18 '24 21:03 jmahlik

https://github.com/mlflow/mlflow/blob/46b7758a9c6cc61f21ddf3a9af8c23c40fd31769/mlflow/utils/requirements_utils.py#L448-L449

Now it's not so rare: uv venv doesn't install setuptools into the environment.

I'm hitting this after doing uv venv with Python 3.11.

astrojuanlu avatar Apr 30 '24 09:04 astrojuanlu

The fact that import mlflow doesn't work out-of-the-box in a clean Python 3.12 virtual environment makes me wonder: is 3.12 explicitly supported by mlflow? Is mlflow tested on 3.12? Is there an official matrix anywhere that will tell me which mlflow versions are safe to run in production against which Python versions?

I see that the pyproject.toml file in the master branch only lists one Python version trove classifier: Programming Language :: Python :: 3.8 -- I imagine that this does not mean that Python>=3.9 is unsupported, but it also doesn't help the reader identify which Python versions are explicitly supported/tested against/etc.

EDIT: Ok, I just realized this issue is not Python 3.12-specific. I thought it was because, for some reason, in my environment (Linux, pyenv, pyenv-virtualenv) this issue only manifests itself in Python 3.12. Anyway, my question about the version matrix still stands.

fofoni avatar May 09 '24 18:05 fofoni

https://github.com/mlflow/mlflow/blob/46b7758a9c6cc61f21ddf3a9af8c23c40fd31769/mlflow/utils/requirements_utils.py#L448-L449

Now it's not so rare: uv venv doesn't install setuptools into the environment.

I'm hitting this after doing uv venv with Python 3.11.

@astrojuanlu I believe the code you're quoting is not used to generate mlflow requirements; instead, it's used at runtime to list the packages required by a given machine learning model.

setuptools is directly used in mlflow, but excluded as a dependency of a package (see these lines).

@TomSteenbergen Same here.

As far as I understand, setuptools is not "removed" from the list of runtime dependencies of mlflow in any way: it simply was never there in the first place.

fofoni avatar May 09 '24 18:05 fofoni

This has to do with setuptools not being added to virtualenvs by default in python 3.12. The what's new and the cpython issue explain the details. Since the requirements_utils module is imported in the __init__.py of mlflow pkg_resources ends up being imported, but it is missing when setuptools isn't present.

It's a tad more difficult than one would think to replace the pkg_resources.working_set since no clear replacement functionality was ever really provided.

jmahlik avatar May 09 '24 18:05 jmahlik