-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix for importing pxr.Tf on windows anaconda 3.8+ interpreters #1642
fix for importing pxr.Tf on windows anaconda 3.8+ interpreters #1642
Conversation
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? |
A |
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 |
If we're worried about mixed conda / non-conda scenarios
Meaning a test build using the stock USD? Or a test build using this PR? |
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. |
I've seen other projects use 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. |
The problem is that the
|
ha, well ok then. Already broken |
317231a
to
1333370
Compare
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, |
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. |
On further thought - the simplest solution might be to not even bother checking for conda, and just ALWAYS modify |
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
1333370
to
9ac848a
Compare
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. |
Filed as internal issue #USD-6948 |
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! |
We are trying to get a conda setup so that we can test, @pmolodo ! |
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__
(andTf.__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 thepxr
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 theCDLL.__init__
code object 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)