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

[Windows] Workaround for loading bundled DLLs #4893

Merged
merged 5 commits into from
Dec 13, 2021

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Nov 10, 2021

Python-3.8+ adds add_dll_directory call, see https://docs.python.org/3/whatsnew/3.8.html#ctypes
Simulate this behaviour on older versions of Python runtime by calling
LoadLibraryExW with the appropriate flags

Fixes #4787

cc @peterjc123 @nbcsm @guyang3532 @maxluk @gunandrose4u @mszhanyi

@facebook-github-bot
Copy link

facebook-github-bot commented Nov 10, 2021

💊 CI failures summary and remediations

As of commit cbf01c9 (more details on the Dr. CI page):


  • 2/2 failures introduced in this PR

2 failures not recognized by patterns:

Job Step Action
CircleCI python_type_check sudo apt-get update -y
sudo apt install -y libturbojpeg-dev
pip install --user --progress-bar off mypy
pip install --user --progress-bar off types-requests
pip install --user --progress-bar off --pre torch -f https://download.pytorch.org/whl/nightly/cpu/torch_nightly.html
pip install --user --progress-bar off git+https://github.com/pytorch/data.git
pip install --user --progress-bar off --no-build-isolation --editable .
mypy --config-file mypy.ini
🔁 rerun
CircleCI binary_libtorchvision_ops_android Build 🔁 rerun

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

torchvision/io/image.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

I can't comment on the actual content. One minor syntax error and a few questions inline.

torchvision/io/image.py Outdated Show resolved Hide resolved
torchvision/io/image.py Outdated Show resolved Hide resolved
torchvision/io/image.py Outdated Show resolved Hide resolved
@malfet malfet force-pushed the malfet/fix-image.pyd-loading-on-windows branch from 4ed6dd3 to 2ee1bd7 Compare November 10, 2021 16:24
torchvision/io/image.py Outdated Show resolved Hide resolved
@andfoy
Copy link
Contributor

andfoy commented Nov 10, 2021

It seems that these changes were also introduced back in PR #2811, do we know why they were removed?

@malfet
Copy link
Contributor Author

malfet commented Nov 10, 2021

It seems that these changes were also introduced back in PR #2811, do we know why they were removed?

Hmm, that's a very good point. Looks like it was accidentally deleted by #4080
cc: @NicolasHug

@malfet malfet force-pushed the malfet/fix-image.pyd-loading-on-windows branch from 2ee1bd7 to 155bd1b Compare November 10, 2021 21:23
@NicolasHug
Copy link
Member

It seems that these changes were also introduced back in PR #2811, do we know why they were removed?

Hmm, that's a very good point. Looks like it was accidentally deleted by #4080 cc: @NicolasHug

Thanks for the ping, and sorry for the trouble. Prior to #4080 we had 3 different registration mechanisms - one for extensions.py, one for image, and one for video. I unified them into a single one - it wasn't clear why each of them were different (no comments, no link to related issues), so I picked the simplest one that seemed to work: the one from extension.py.

@malfet malfet force-pushed the malfet/fix-image.pyd-loading-on-windows branch 2 times, most recently from c0ab563 to cbf01c9 Compare November 10, 2021 23:59
@malfet malfet force-pushed the malfet/fix-image.pyd-loading-on-windows branch from cbf01c9 to 2d8c032 Compare December 13, 2021 17:13
torchvision/io/image.py Outdated Show resolved Hide resolved
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks a lot Nikita, and sorry for the delay in getting back on this.

I have one high-level comment, otherwise the PR is good to merge.

torchvision/_internally_replaced_utils.py Outdated Show resolved Hide resolved
torchvision/_internally_replaced_utils.py Outdated Show resolved Hide resolved
@malfet
Copy link
Contributor Author

malfet commented Dec 13, 2021

Moved _load_library to torchvision.extension

Comment on lines 1 to 3
import ctypes
import os
import sys
Copy link
Member

Choose a reason for hiding this comment

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

nit: you probably need to move those imports to extension.py now

@malfet malfet force-pushed the malfet/fix-image.pyd-loading-on-windows branch 3 times, most recently from 6741c31 to d93ac2c Compare December 13, 2021 19:36
Python-3.8+ adds `add_dll_directory` call, see https://docs.python.org/3/whatsnew/3.8.html#ctypes
Simulate this behaviour on older versions of Python runtime by calling
`LoadLibraryExW` with the appropriate flags

Fixes #4787
Get rid of `pass`, import `os`, `sys` and `warnnings` globally, fix
`global()`->`globals()` typo
@malfet malfet force-pushed the malfet/fix-image.pyd-loading-on-windows branch from d93ac2c to 52a6e14 Compare December 13, 2021 19:38
@malfet malfet merged commit 9f48c62 into main Dec 13, 2021
@malfet malfet deleted the malfet/fix-image.pyd-loading-on-windows branch December 13, 2021 21:58
@github-actions
Copy link

Hey @malfet!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

malfet added a commit that referenced this pull request Dec 13, 2021
* [Windows] Workaround for loading bundled DLLs

Python-3.8+ adds `add_dll_directory` call, see https://docs.python.org/3/whatsnew/3.8.html#ctypes
Simulate this behaviour on older versions of Python runtime by calling
`LoadLibraryExW` with the appropriate flags

Fixes #4787
facebook-github-bot pushed a commit that referenced this pull request Dec 21, 2021
Summary:
* [Windows] Workaround for loading bundled DLLs

Python-3.8+ adds `add_dll_directory` call, see https://docs.python.org/3/whatsnew/3.8.html#ctypes
Simulate this behaviour on older versions of Python runtime by calling
`LoadLibraryExW` with the appropriate flags

Fixes #4787

Reviewed By: prabhat00155

Differential Revision: D33253476

fbshipit-source-id: bc462bf1c3b161646b258abc495c46f32aa1b459
pass
_load_library("image")
except (ImportError, OSError) as e:
warn(f"Failed to load image Python extension: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that this was changed from pass to warn? When running pytest on my project, I see warnings like:

../.spack/.spack-env/view/lib/python3.9/site-packages/torchvision/io/image.py:11
  /Users/Adam/.spack/.spack-env/view/lib/python3.9/site-packages/torchvision/io/image.py:11: UserWarning: Failed to load image Python extension: 
    warn(f"Failed to load image Python extension: {e}")

It's unclear if this is a serious bug in my installation that I should fix, or something I can safely ignore. FWIW, I'm using the pillow-SIMD backend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automated testing discovered that image::read_png sometimes can not be initialized on Windows
7 participants