Skip to content

Conversation

@anthonyalayo
Copy link
Contributor

@anthonyalayo anthonyalayo commented Oct 2, 2023

This is a re-attempt of fixing #53980, first submitted in #54978.

Quoting @SpaceIm

Fixes https://github.com/pytorch/pytorch/issues/53980

Maybe it would be nice to find why some files are generated in CMAKE_BINARY_DIR instead of CMAKE_CURRENT_BINARY_DIR or Torch_BINARY_DIR or PROJECT_BINARY_DIR, but there is a lot of indirection in the logic of pytorch build files, so I was not able to find where it comes from.

cc @mcarilli @ptrblck @leslie-fang-intel @jgong5

@pytorch-bot
Copy link

pytorch-bot bot commented Oct 2, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/110373

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit 58408c2 with merge base 06464a3 (image):

UNSTABLE - The following job failed but was likely due to flakiness present on trunk and has been marked as unstable:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@anthonyalayo
Copy link
Contributor Author

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Oct 2, 2023
@anthonyalayo
Copy link
Contributor Author

@pytorchbot drci

@anthonyalayo
Copy link
Contributor Author

@malfet I wanted to revive this PR since it would be great to have, originally here:
#54978

Any idea why these CI checks are failing? I'm looking at them, but nothing looks related?

I tested this in a project I work on and it looked good there.

@anthonyalayo
Copy link
Contributor Author

cc @albanD who helped me get my last PyTorch contribution in

@malfet
Copy link
Contributor

malfet commented Oct 2, 2023

It would be nice to add some trivial unittest (that just runs cmake on a project that lists pytorch as a subproject)

@malfet
Copy link
Contributor

malfet commented Oct 2, 2023

@pytorchbot rebase

@malfet
Copy link
Contributor

malfet commented Oct 2, 2023

Any idea why these CI checks are failing? I'm looking at them, but nothing looks related?

@anthonyalayo your branch is 3000 commits behind main, please rebase (if mergebot fails to do it for you)
image

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased aalayo-fix/cmake-allow-addsubdirectory-fetchcontent onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout aalayo-fix/cmake-allow-addsubdirectory-fetchcontent && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the aalayo-fix/cmake-allow-addsubdirectory-fetchcontent branch from 677dab7 to 58408c2 Compare October 2, 2023 19:12
@anthonyalayo
Copy link
Contributor Author

@anthonyalayo your branch is 3000 commits behind main, please rebase (if mergebot fails to do it for you)

Nice catch, didn't realize I didn't update my fork.

It would be nice to add some trivial unittest (that just runs cmake on a project that lists pytorch as a subproject)

I'll do that next, thanks @malfet

@anthonyalayo
Copy link
Contributor Author

@pytorchbot drci

@colesbury colesbury requested a review from malfet October 3, 2023 23:02
@colesbury colesbury added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 3, 2023
@anthonyalayo
Copy link
Contributor Author

anthonyalayo commented Oct 4, 2023

@malfet I started writing the test, but it takes a pretty long time to fetch and build since it pulls in PyTorch itself. Is that alright to have as something that runs on CI?

I'm basing the test off of this one:
4972cf0

Unlike that one though, since we are testing that ExternalProject_Add() works, we can't leverage the build that gets generated through the python test script.

@anthonyalayo
Copy link
Contributor Author

@malfet any thoughts on that last comment?

@malfet malfet added ciflow/binaries Trigger all binary build and upload jobs on the PR ciflow/trunk Trigger trunk jobs on your pull request labels Oct 6, 2023
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Changes to the 3 CMakeLists.txt and Codegen.cmake looks fine to me

@malfet
Copy link
Contributor

malfet commented Oct 6, 2023

@anthonyalayo thank you for the updates, looks good to me if trunk and binary builds are passing.
As for the test, it can be done as followup PR(because otherwise, if not tested it will regress after a while), but yes, something similar to #41145 would do

@anthonyalayo
Copy link
Contributor Author

Looks like everything test clean, good to merge?

@anthonyalayo
Copy link
Contributor Author

@malfet can we start the merge? thanks!

@anthonyalayo
Copy link
Contributor Author

@malfet any thoughts?

@malfet
Copy link
Contributor

malfet commented Oct 10, 2023

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

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

Labels

ciflow/binaries Trigger all binary build and upload jobs on the PR ciflow/trunk Trigger trunk jobs on your pull request Merged open source topic: not user facing topic category triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants