Remove Python linking dependency if USE_PYTHON=OFF

[laitingsheng]

A similar fix proposed previously by #5602 failed the CI tests. I made some amendments, and it is compilable in my environment.

It should now correct the problem raised in #5863 as well.

Closes #5602.
Resolves #5863.

[facebook-github-bot]

Hi @laitingsheng!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[datumbox]

The approach looks reasonable.

@fmassa Any thoughts around this?

[fmassa]

LGTM if tests passes!

Also, ccing @bmanga as you have originally implemented the cmake file

[fmassa]

Also, might be worth seeing if we can remove the following lines

vision/torchvision/csrc/vision.cpp

Lines 17 to 27 in 2e70ee1

	// If we are in a Windows environment, we need to define
	// initialization functions for the _custom_ops extension.
	// For PyMODINIT_FUNC to work, we need to include Python.h
	#if !defined(MOBILE) && defined(_WIN32)
	#ifdef USE_PYTHON
	PyMODINIT_FUNC PyInit__C(void) {
	// No need to do anything.
	return NULL;
	}
	#endif // USE_PYTHON
	#endif // !defined(MOBILE) && defined(_WIN32)

since this is what blocks us to have to include python (but this wasn't supposed to be the case). Maybe we can now remove this, or folks from torchaudio have found a different workaround since then? cc @mthrok

[datumbox]

@fmassa Thanks a lot for offering an other pair of eyes.

@laitingsheng My intention is to wait a couple of days for the rest of the people to respond and merge afterwards. Please don't close the PR. Thanks in advance!

[laitingsheng]

My intention is to wait a couple of days for the rest of the people to respond and merge afterwards. Please don't close the PR. Thanks in advance!

My pleasure.

Right now our CI is still broken due to PyTorch upstream issues, so it's not a good time to test it. Let's wait until they are resolved (tracked at #5881) and then check what the CI things about them.

The quote is from comments under #5602. This is surprising to me as the tests failed again. Since I have permission to look at the CircleCI logs for my commit, I reckon this might be an issue due to the CI system environment. I guess the docker image cimg/python eliminates the shared object files of LibPNG to reduce the overall size, but I don't have a chance to check this. I suggest adding a preparation step like apt-get install libpng-dev to the CircleCI configuration.

[datumbox]

@laitingsheng So in the previous commit, for some reason, the CI didn't run all the tests. I'm certain because I checked so I kicked the CI to rerun them.

Unfortunately the failings are related so to avoid accidental merges, I'll mark the PR with "Request Changes". If we can make the CI pass, I'll be happy to unblock. Apologies for the inconvenience.

[laitingsheng]

I found a related issue in python-pillow/Pillow#5813.

I think the error indicates that the shared object path resolution might be broken in the testing environment, but adding the Python target to the linkage dependency somehow resolves this issue. The last commit I pushed might solve the problem on CEntOS-based testing environments, but not on those running macOS.

From my perspective, it might be necessary to prune the whole building and testing scripts to actually resolve this issue, but this involves a large amount of work. I will now close my PR and see if any smarter coders could propose a better solution instead.

[datumbox]

@laitingsheng Understood, thanks for the effort!