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.

Sign up for GitHub

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

Jump to bottom

Conditionally include Python in the C++ builds. #5190

Merged
merged 14 commits into from
Jan 17, 2022
Merged

Conditionally include Python in the C++ builds. #5190

merged 14 commits into from
Jan 17, 2022

Conversation

dfalbel
Copy link
Contributor

@dfalbel dfalbel commented Jan 11, 2022 •
edited
Loading

@fmassa Thanks for your suggestions and links in #2692 (comment)

This PR adds an option USE_PYTHON that allows users to decide to link to Python3 or not.
We made it OFF by default as suggest in #2692 (comment).
We might still need to add the definitions for USE_PYTHON so it gets linked in the Windows build mentioned in #3965 (comment) (that should be done now)

This should fix #2692

Let me know if this makes sense. Thanks again!

@dfalbel
Conditionally include Python in the C++ builds.
e9ec758 
Added an option `USE_PYTHON` that allows users to decide to link to Python3 or not.
@facebook-github-bot
Copy link

facebook-github-bot commented Jan 11, 2022 •
edited
Loading

💊 CI failures summary and remediations

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

✅ None of the CI failures appear to be your fault 💚

🚧 1 ongoing upstream failure:

These were probably caused by upstream breakages that are not fixed yet.

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.

@dfalbel dfalbel mentioned this pull request Jan 11, 2022
Closed
datumbox
datumbox reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Overall looks good. Thanks @dfalbel, I've added only one comment for putting some documentation around the flag.

Before merging this, is worth checking on FBcode side to see if it breaks anything or if changes will be needed on BUCK. @prabhat00155 I know you already did the syncing this week, but would you have the bandwidth to cherrypick and test this PR internally to confirm everything works as expected?

CMakeLists.txt Show resolved Hide resolved
@dfalbel
Copy link
Contributor Author

dfalbel commented Jan 12, 2022 •
edited
Loading

Thanks @datumbox ! If something fails I think we can still decide to make USE_PYTHON=on by default so as to keep the current behavior while allowing one to disable Python linking.

prabhat00155
prabhat00155 approved these changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@prabhat00155 prabhat00155 left a comment

Choose a reason for hiding this comment

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

Thanks @dfalbel!

fmassa
fmassa approved these changes Jan 17, 2022
View reviewed changes
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.

Changes LGTM, thanks!

For a future PR (which will need FB access), it would be good to remove the MOBILE var, as it can now be used through USE_PYTHON. It will need changes to fbcode and xplat I believe.

@dfalbel dfalbel mentioned this pull request Jan 17, 2022
Closed
@prabhat00155 prabhat00155 merged commit 5e56575 into pytorch:main Jan 17, 2022
@github-actions
Copy link

Hey @prabhat00155!

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

@prabhat00155
Copy link
Contributor

Hey @prabhat00155!

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

@NicolasHug Should we add "build" to SECONDARY LABELS?

@NicolasHug
Copy link
Member

sure, thanks for flagging

facebook-github-bot pushed a commit that referenced this pull request Jan 19, 2022
@prabhat00155 @facebook-github-bot
[fbsync] Conditionally include Python in the C++ builds. (#5190)
bf89752 
Summary:
* Conditionally include Python in the C++ builds.

Added an option `USE_PYTHON` that allows users to decide to link to Python3 or not.

* Also conditionally include this defintion.

* Define `USE_PYTHON` for the Windows builds.

* Remove Python3 reference in CMake test as it's no longer required.

* Accidentally removed the closing > from the decl.

* Also add `USE_PYTHON` when building the separate image library on Windows.

* Also conditionally include this depening on USE_PYTHON.

* Go back to require Python for this example.

* Find Python in the hello world example.

* Add a paragraph documenting the `USE_PYTHON` option.

Reviewed By: datumbox, NicolasHug

Differential Revision: D33655257

fbshipit-source-id: 34828b4f0c14e700ac19a6ee6dbf5d26e9fa76ca

Co-authored-by: Prabhat Roy <prabhatroy@fb.com>
@prabhat00155 prabhat00155 mentioned this pull request Jan 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datumbox datumbox datumbox left review comments

@fmassa fmassa fmassa approved these changes

@prabhat00155 prabhat00155 prabhat00155 approved these changes

Assignees
No one assigned
Labels
ciflow/default cla signed enhancement topic: build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Using torchvision C++ API without installing python
6 participants
@dfalbel @facebook-github-bot @prabhat00155 @NicolasHug @datumbox @fmassa