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

fix for importing pxr.Tf on windows anaconda 3.8+ interpreters #1642

Merged

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Oct 4, 2021

cpython changed the way the dlls are loaded on windows for python-3.8+. USD implemented a workaround for this change, installed into pxr.__init__ (and Tf.__init__). However, anaconda python interpreters were patched to behave more like pre-3.8:

https://github.com/conda-forge/python-feedstock/blob/8195ba1178041b7461238e8c5680eee62f5ea9d0/recipe/patches/0020-Add-CondaEcosystemModifyDllSearchPath.patch#L37

Note: I'm not crazy about adding code that does a filesystem operation (os.path.exists) when the pxr module is imported. However, it seems to be the most robust way to check for conda. The best alternative I could come up with was to search through the CDLL.__init__ code object varnames:

import ctypes
is_conda = 'LOAD_WITH_ALTERED_SEARCH_PATH' in ctypes.CDLL.__init__.__code__.co_varnames

...but for some reason that feels more fragile to me. I can switch to use that approach if you'd prefer, though.

Description of Change(s)

We now check for anaconda interpreters before using the 3.8+ only fix.

Fixes Issue(s)

@meshula
Copy link
Member

meshula commented Oct 5, 2021

This will accommodate conda, if conda is in the path, but overridden by another python with higher path-precedence. I was debugging an issue around that exact scenario today... What about adding a --conda option to build_usd.py itself to remove any heuristics, and relying on the end user to supply the information instead?

@sunyab
Copy link
Contributor

sunyab commented Oct 5, 2021

A --conda option won't work for PyPI packages since (AFAIK) there's no difference between the package downloaded by pip in a conda environment vs. a standard environment.

@meshula
Copy link
Member

meshula commented Oct 5, 2021

I should have read the linked issue :) My mind went to running the build_usd.py script under conda. FWIW the test build that I did while not reading #1602 succeeded under conda/3.9 LOL

@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 5, 2021

This will accommodate conda, if conda is in the path, but overridden by another python with higher path-precedence.

If we're worried about mixed conda / non-conda scenarios PYTHONPATH scenarios - then maybe we should use the ctypes.CDLL check I mentioned above? Since that's more directly related to the change we're interested in...

FWIW the test build that I did while not reading #1602 succeeded under conda/3.9 LOL

Meaning a test build using the stock USD? Or a test build using this PR?

@meshula
Copy link
Member

meshula commented Oct 5, 2021

Yes, a test build using stock USD, under conda, from top of dev branch without a patch.

The reason I am concerned about the mixed conda environment is due to the fact that on mac the system has always got a python 2.7 lurking in the path. Normally it's completely masked by conda in the path of course, but a shell script I encountered today referenced /usr/bin/python by full path, and things got weird.

This sort of scenario has to be much less problematic on Windows, since there's no one magic place to rely on for a canonical python installation.

@rstelzleni
Copy link
Contributor

I've seen other projects use 'conda' in sys.version to detect when they're running in a conda python interpreter. The stackoverflow thread linked above says that might be fragile because they could rename their version string, but it seems like they could just as easily change their build setup to not create a folder named conda-meta.

Side note, it might be good to add a test to the pypi azure pipeline that uses a conda environment instead of a default installed python.

BUILDING.md Outdated Show resolved Hide resolved
@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 5, 2021

I've seen other projects use 'conda' in sys.version to detect when they're running in a conda python interpreter. The stackoverflow thread linked above says that might be fragile because they could rename their version string, but it seems like they could just as easily change their build setup to not create a folder named conda-meta.

The problem is that the 'conda' in sys.version check is already broken - in my conda 3.9.7, this is my sys.version:

'3.9.7 (default, Sep 16 2021, 16:59:28) [MSC v.1916 64 bit (AMD64)]'

@rstelzleni
Copy link
Contributor

'3.9.7 (default, Sep 16 2021, 16:59:28) [MSC v.1916 64 bit (AMD64)]'

ha, well ok then. Already broken

@pmolodo pmolodo force-pushed the pr/fix_anaconda_py_38_39 branch 2 times, most recently from 317231a to 1333370 Compare October 5, 2021 21:17
@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 5, 2021

So, have another potential way of checking for conda:

import os
import sys
CONDA_ENV_VAR = "CONDA_DLL_SEARCH_MODIFICATION_ENABLE"

def is_windows_conda():
    try:
        import _winapi
        lib_path = _winapi.GetModuleFileName(sys.dllhandle)
        with open(lib_path, "rb") as f:
            lib_bytes = f.read()
    except Exception:
        return False
    # windows uses utf-16 encoding in it's binaries, with leading byte-order
    # mark stripped off
    env_var_bytes = CONDA_ENV_VAR.encode("utf16")[2:]
    return env_var_bytes in lib_bytes

def using_py38_dll_loading():
    return (sys.version_info >= (3, 8, 0) and (not is_windows_conda()
                                               or CONDA_ENV_VAR in os.environ))

print(using_py38_dll_loading())

This checks, ie, python39.dll for the binary string of CONDA_DLL_SEARCH_MODIFICATION_ENABLE, which should obviously only be there for conda-modified interpreters. The advantage of this is that is a value that is unlikely to change, as doing so would break code for those relying on it, and we're checking for something directly tied to the feature we care about. It should also be fairly accurate, even in "mixed path" scenarios (such as when sys.executable and the underlying python library are coming from different places, which I've seen!) Downside is that we have to rely on details like how windows encodes strings into dlls. Also, we're relying on being able to read the dll from the filesystem - so, for instance, if the entire python install is zipped, this won't work. But that would also cause problems for the conda-meta directory check.

@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 5, 2021

I modified the PR to use the above method of checking for conda, since I think it's the best option thus far. Let me know if you disagree - I don't feel too strongly about this.

@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 5, 2021

On further thought - the simplest solution might be to not even bother checking for conda, and just ALWAYS modify PATH... and then additionally set PXR_USD_WINDOWS_DLL_PATH if we're >= 3.8. The advantage here is this should "just work" regardless of which python interpreter we're using - since extra calls to os.add_dll_directory shouldn't do anything to the conda interpreter, and the extra add to the PATH won't hurt non-conda 3.8 (I mean, if it didn't do any harm for <3.7, it shouldn't hurt now. You could even make the case that by continuing to add to the path, we're keeping more consistent behavior, if for some reason someone was relying on some other binary they've added to that folder.)

cpython changed the way the dlls are loaded on windows for python-3.8+.
USD implemented a workaround for this change, installed into
pxr.__init__ (and Tf.__init__).  However, anaconda python interpreters
were patched to behave more like pre-3.8:

   https://github.com/conda-forge/python-feedstock/blob/8195ba1178041b7461238e8c5680eee62f5ea9d0/recipe/patches/0020-Add-CondaEcosystemModifyDllSearchPath.patch#L37

So we now check for anaconda interpreters before using the 3.8+ only
fix.

Fixes issue PixarAnimationStudios#1602
@pmolodo
Copy link
Contributor Author

pmolodo commented Oct 5, 2021

Ok - did another push where I just went with the always-modify-the PATH approach. I think it's the safest, and it's definitely the simplest solution.

@sunyab
Copy link
Contributor

sunyab commented Oct 5, 2021

Filed as internal issue #USD-6948

@pmolodo
Copy link
Contributor Author

pmolodo commented Nov 11, 2021

Hi - now that 21.11 is out the door, just wanted to ping here to see if we could get this into the next release. Thanks!

@spiffmon
Copy link
Member

We are trying to get a conda setup so that we can test, @pmolodo !

@pixar-oss pixar-oss merged commit 4ce6f19 into PixarAnimationStudios:dev Dec 18, 2021
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.

6 participants