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

[bazel] change visibility for //c10:headers #91422

Closed
wants to merge 2 commits into from

Conversation

vors
Copy link
Contributor

@vors vors commented Dec 27, 2022

At Cruise we are actively depending on the c10 headers, I'm not certain what is the reason to hide them to the pkg level.

@pytorch-bot
Copy link

pytorch-bot bot commented Dec 27, 2022

🔗 Helpful Links

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

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

✅ No Failures

As of commit 722d3bd:
💚 Looks good so far! There are no failures yet. 💚

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

@zou3519 zou3519 requested a review from a team January 4, 2023 15:08
@zou3519 zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jan 4, 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.

Hmm, looks like it legitimately breaks bazel build, but imo that's because default visibility is private and it should be public, shouldn't it?

ERROR: /var/lib/jenkins/workspace/BUILD.bazel:1281:11: in cc_library rule //:caffe2_for_aten_headers: target '//c10:headers' is not visible from target '//:caffe2_for_aten_headers'. Check the visibility declaration of the former target if you think the dependency is legitimate

If this passes the build (sorry, can't make suggestions on deleted line), will merge it: #91715

@vors
Copy link
Contributor Author

vors commented Jan 4, 2023

Oh lol, I must be too used to our setup. Let me fix it, thank you.

@malfet malfet added release notes: build release notes category topic: improvements topic category labels Jan 4, 2023
@malfet
Copy link
Contributor

malfet commented Jan 4, 2023

Thank you for the quick fix, looks good to me.

@dagitses
Copy link
Collaborator

dagitses commented Jan 4, 2023

Why are you depending on the headers rather than the full library?

@malfet
Copy link
Contributor

malfet commented Jan 4, 2023

Why are you depending on the headers rather than the full library?

I wonder why :headers library is there the first place, and why say :caffe2_for_aten_headers depends on //c10:headers?

@malfet
Copy link
Contributor

malfet commented Jan 4, 2023

@pytorchbot merge -f "Bazel cpu builds passing"

@dagitses
Copy link
Collaborator

dagitses commented Jan 4, 2023

Why are you depending on the headers rather than the full library?

I wonder why :headers library is there the first place, and why say :caffe2_for_aten_headers depends on //c10:headers?

Agreed. It really only exists because the Bazel build has been pretty haphazard with defining libraries of headers. I would like to see this cleaned up, and having an external user depending on this is not ideal for that goal.

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@malfet
Copy link
Contributor

malfet commented Jan 4, 2023

Why are you depending on the headers rather than the full library?

I wonder why :headers library is there the first place, and why say :caffe2_for_aten_headers depends on //c10:headers?

Agreed. It really only exists because the Bazel build has been pretty haphazard with defining libraries of headers. I would like to see this cleaned up, and having an external user depending on this is not ideal for that goal.

I thought that c10 build definitions are fully unified between between internal buck and public bazel build systems, aren't they?

@vors
Copy link
Contributor Author

vors commented Jan 4, 2023

I don't think that c10 target exposes the headers (it doesn't depend on it) https://github.com/pytorch/pytorch/blob/master/c10/build.bzl

Also these queries show that

sergei.vorobev@cs-7xn77uoy-gpu-homedir-848671:~/workspaces/c2/src/github.robot.car/cruise/cruise$ bazel query 'somepath(@pytorch//c10, @pytorch//c10:headers)'
WARNING: build volume is nearly full (104.165 GB remain). Free up space to avoid running out of disk!

INFO: Invocation ID: ddea5aba-8c65-11ed-b684-42010ad8716a
INFO: Empty results
Loading: 18 packages loaded
sergei.vorobev@cs-7xn77uoy-gpu-homedir-848671:~/workspaces/c2/src/github.robot.car/cruise/cruise$ bazel query 'somepath(@pytorch//c10:headers, @pytorch//c10)'
WARNING: build volume is nearly full (104.165 GB remain). Free up space to avoid running out of disk!

INFO: Invocation ID: e3970a9e-8c65-11ed-b684-42010ad8716a
INFO: Empty results
Loading: 0 packages loaded

@vors
Copy link
Contributor Author

vors commented Jan 5, 2023

I filled #91751 to track improving this c10:headers situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merged open source release notes: build release notes category topic: improvements 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.

None yet

6 participants