Skip to content
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

Prefer site-packages paths with Python version #35

Merged

Conversation

stefanv
Copy link
Member

@stefanv stefanv commented Dec 19, 2022

On Linux, you get python3.6/site-packages or python3/site-packages. This helps to ambiguate these paths for different versions of Python.

On Windows, it is typically \usr\Lib\site-packages, so we cannot rely on the version being present.

On Linux, you get python3.6/site-packages or python3/site-packages.
This helps to ambiguate these paths for different versions of Python.

On Windows, it is typically \usr\Lib\site-packages, so we cannot rely on
the version being present.
@mattip
Copy link
Contributor

mattip commented Dec 20, 2022

Is the version a concern? The c-extension modules are version-specific or abi3 which should be loaded by any version, right?

@mattip
Copy link
Contributor

mattip commented Dec 20, 2022

Should there be a check that there is not more than one valid path?

@stefanv
Copy link
Member Author

stefanv commented Dec 20, 2022

@mattip The version is a concern if I build numpy for py311 and py310 side-by-side. What do you think is the logic for only one valid path? Perhaps filter the list using python3.10, if that doesn't exist, filter by python3, if that doesn't exist just return whatever is available? That was essentially what the sorting achieved, but it doesn't help you in the case where there is only a python3.11 build and you are running on py310.

@mattip
Copy link
Contributor

mattip commented Dec 20, 2022

The version is a concern if I build numpy for py311 and py310 side-by-side

Why? What is the concern? The python files are identical, and the c-extensions have different names. I understand meson will create two directories on linux: build-install/usr/lib/python3.10/site-packages and build-install/usr/lib/python3.11/site-packages. One windows it will create a single build-install/usr/Lib/site-packages. So I think devpy should add the appropriate path to sys.path. There should never be two matches for the python under test.

@mattip
Copy link
Contributor

mattip commented Dec 20, 2022

Perhaps filter the list using python3.10, if that doesn't exist, filter by python3, if that doesn't exist just return whatever is available?

If that doesn't exist, error "something is wrong"

in the case where there is only a python3.11 build and you are running on py310.

I would prefer a clear error when testing starts and not a "cannot import" error later on

@stefanv
Copy link
Member Author

stefanv commented Dec 21, 2022

But do we know what the path will look like on different platforms? Windows has no version info, Python seems to add major and sometimes minor version, not sure about MacOS?

@mattip
Copy link
Contributor

mattip commented Dec 21, 2022

I think I would prefer this, does it make sense?:

def get_site_packages(build_dir):
    candidate_paths = []
    for root, dirs, files in os.walk(install_dir(build_dir)):
        for subdir in dirs:
            if subdir == "site-packages":
                candidate_paths.append(os.path.abspath(os.path.join(root, subdir)))
    if len(candidate_paths) == 0:
        raise ValueError(f"cannot find any 'site-packages' in {build_dir}")
    if len(candidate_paths) == 1:
        return candidate_paths[0]
    # Find paths with current Python version in them
    X, Y = sys.version_info.major, sys.version_info.minor
    major_paths = [p for p in candidate_paths if f"python{X}" in candidate_paths.split(os.sep)]
    if len(major_paths) > 1:
        raise ValueError(f"cannot find proper 'site-packages' to use in {build_dir}, more than one 'python{X}'")
    if len(major_paths == 1:
        return major_paths[0]
    minor_paths = [p for p in candidate_paths if f"python{X}.{Y}" in candidate_paths.split(os.sep)]
    if len(minor_paths) > 1:
        raise ValueError(f"cannot find proper 'site-packages' to use in {build_dir}, more than one 'python{X}.{Y}'")
    if len(minor_paths == 1:
        return minor_paths[0]
    raise ValueError(f"cannot find any 'site-packages' in {build_dir} suitable for python{X}.{Y}")

@rgommers
Copy link
Contributor

I recommend copying the logic from SciPy's dev.py exactly. It's reliable now AFAIK and was quite difficult to get right, there are lots of platform-specific variations.

I am seeing this problem locally as well now for some reason, where before it used to work. Not sure what changed, maybe Python version or some such thing.

@stefanv
Copy link
Member Author

stefanv commented Dec 21, 2022

I recommend copying the logic from SciPy's dev.py exactly. It's reliable now AFAIK and was quite difficult to get right, there are lots of platform-specific variations.

That logic relies on distutils, and given this comment I didn't think that was the right approach:

# distutils is required to infer meson install path
# if this needs to be replaced for Python 3.12 support and there's no
# stdlib alternative, use CmdAction and the hack discussed in gh-16058

Directly searching for the path seems like a simpler alternative, if we can make it work?

I am seeing this problem locally as well now for some reason, where before it used to work. Not sure what changed, maybe Python version or some such thing.

Perhaps you're on the previous version still? On main the path search has been reverted to its old behavior, and works the way it always did.

@stefanv
Copy link
Member Author

stefanv commented Dec 21, 2022

I think I would prefer this, does it make sense?:

That looks correct, except that the more specific check (3.10) needs to come before the less specific check (3).

@stefanv
Copy link
Member Author

stefanv commented Dec 23, 2022

@mattip How about this version?

The exact heuristics that need to be replicated are the ones from mesonpy, I suppose.

The logic here is simpler, because we can use the installed files to help guide us.

  1. Search for any site-packages or dist-packages.
  2. If the results contain "python3.X", it means that the paths encode the major/minor version. In this case, ensure that our Python version is matched.
  3. If not, we cannot distinguish between products for different versions of Python, so just return any site-packages path found.

site_packages = [p for p in candidate_paths if f"python{X}.{Y}" in p]
if len(site_packages) == 0:
print(f"No site-packages found in `{build_dir}` for Python {X}.{Y}")
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems wrong for a utility function to call sys.exit. I would prefer it raise and the script runner catch the exception and exit

# A naming scheme that does not encode the Python major/minor version is used, so return
# the first site-packages path found
if len(candidate_paths) != 0:
site_packages = candidate_paths[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

This should check that only one such directory exists

@mattip
Copy link
Contributor

mattip commented Dec 23, 2022

No tests?

@stefanv
Copy link
Member Author

stefanv commented Dec 23, 2022

I'll add tests and see if there's a convenient way to change exiting to exceptions, thanks.

@mattip
Copy link
Contributor

mattip commented Jan 3, 2023

Any progress?

@stefanv
Copy link
Member Author

stefanv commented Jan 4, 2023

Sorry, I was a bit pre-occupied with the annual festive transition; back in the saddle now!

@stefanv stefanv self-assigned this Jan 10, 2023
@stefanv stefanv force-pushed the prefer-site-path-with-python-ver branch 2 times, most recently from c50c7c8 to e35f35c Compare January 11, 2023 01:34
@stefanv stefanv force-pushed the prefer-site-path-with-python-ver branch from e35f35c to 322fc02 Compare January 11, 2023 01:38
@stefanv
Copy link
Member Author

stefanv commented Jan 11, 2023

OK @mattip this should do the trick!

f"/usr/lib64/python{X}.{Y + 2}/site-packages/",
],
)
util.get_site_packages(build_dir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this check that the returned value is the one expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it should! Done.

@mattip
Copy link
Contributor

mattip commented Jan 11, 2023

Thanks. Looks good, maybe a bit of test improvement possible but fine to do that in subsequent PRs. As this matures, it would be good to add a version and put it on PyPI

@stefanv
Copy link
Member Author

stefanv commented Jan 11, 2023

I added a test for return values. I will add testing on Windows in another PR; it may require a bit of tweaking to get right.

@stefanv stefanv merged commit b66ca5c into scientific-python:main Jan 11, 2023
@mattip
Copy link
Contributor

mattip commented Jan 11, 2023

Thanks

@jarrodmillman jarrodmillman added this to the 0.1 milestone Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants