Skip to content

Warn if torchvision imported from repo root #2759

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

Merged
merged 3 commits into from
Oct 12, 2020

Conversation

jamt9000
Copy link
Contributor

@jamt9000 jamt9000 commented Oct 6, 2020

Suggested fix for #2239

(Should it be an error or are there cases where it would be desired during development?)

@jamt9000 jamt9000 force-pushed the import_in_root_warning branch from 6b9d93b to 9af65a7 Compare October 6, 2020 00:11
This is likely to fail as mentioned in pytorch#2239
@jamt9000 jamt9000 force-pushed the import_in_root_warning branch from 9af65a7 to e72c037 Compare October 6, 2020 00:12
@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #2759 into master will decrease coverage by 0.25%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2759      +/-   ##
==========================================
- Coverage   73.22%   72.97%   -0.26%     
==========================================
  Files          96       96              
  Lines        8431     8325     -106     
  Branches     1317     1296      -21     
==========================================
- Hits         6174     6075      -99     
+ Misses       1857     1850       -7     
  Partials      400      400              
Impacted Files Coverage Δ
torchvision/__init__.py 80.55% <100.00%> (+11.80%) ⬆️
torchvision/extension.py 48.64% <0.00%> (-19.54%) ⬇️
torchvision/io/video.py 69.82% <0.00%> (-10.66%) ⬇️
torchvision/io/image.py 80.00% <0.00%> (-9.14%) ⬇️
torchvision/ops/deform_conv.py 66.66% <0.00%> (-4.31%) ⬇️
torchvision/ops/boxes.py 93.25% <0.00%> (-2.15%) ⬇️
torchvision/ops/roi_align.py 68.96% <0.00%> (-2.01%) ⬇️
torchvision/ops/roi_pool.py 73.07% <0.00%> (-1.93%) ⬇️
torchvision/ops/ps_roi_pool.py 73.07% <0.00%> (-1.93%) ⬇️
torchvision/ops/ps_roi_align.py 71.42% <0.00%> (-1.91%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 831c0df...bff8a52. Read the comment docs.

@andfoy
Copy link
Contributor

andfoy commented Oct 6, 2020

Hi @jamt9000, thanks for this contribution! My only comment here is that we should rather check for the existence of the compiled files _C.so in both Mac and Linux and _C.pyd on Windows, since that shared library is the one that might cause failures when is not found.

@andfoy
Copy link
Contributor

andfoy commented Oct 6, 2020

Maybe you can use this call to importlib to find it:

extfinder = importlib.machinery.FileFinder(lib_dir, loader_details) # type: ignore[arg-type]

@jamt9000
Copy link
Contributor Author

jamt9000 commented Oct 6, 2020

@andfoy It looks like torchvision checks for _C.so already

extfinder = importlib.machinery.FileFinder(lib_dir, loader_details)
ext_specs = extfinder.find_spec("_C")
if ext_specs is None:
raise ImportError

try:
_register_extensions()
_HAS_OPS = True
except (ImportError, OSError):
pass

and can continue with reduced functionality if it is missing.

Maybe it should always show a warning when _HAS_OPS = False (which could be for several reasons) and then also
show an extra message if the reason for this seems to be because of being in the root dir.

@fmassa
Copy link
Member

fmassa commented Oct 6, 2020

@jamt9000 yes, adding checks for the existence of _HAS_OPS is something that we are considering doing, see for example #2467 (which makes me think that I should revamp that PR, I completely forgot about it)

@fmassa
Copy link
Member

fmassa commented Oct 6, 2020

and I agree with @andfoy , we should instead check for the existence of the _C library.
I for example always run torchvision from the root folder, but I install it via python setup.py build develop, so it works fine. Also a few other systems might package things weirdly, so it's better to error on the safe side.

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.

LGTM, thanks a lot!

@mthrok @cpuhrsch this is something that could be interesting to be done to the other libraries as well. Also, I'm not sure if the current implementation works as expected in fbcode or if we need to do something else, but let's see.

@fmassa fmassa merged commit 42e7f1f into pytorch:master Oct 12, 2020
bryant1410 pushed a commit to bryant1410/vision-1 that referenced this pull request Nov 22, 2020
* Warn if torchvision imported from repo root

This is likely to fail as mentioned in pytorch#2239

* Only warn if ops are missing
vfdev-5 pushed a commit to Quansight/vision that referenced this pull request Dec 4, 2020
* Warn if torchvision imported from repo root

This is likely to fail as mentioned in pytorch#2239

* Only warn if ops are missing
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.

3 participants