Skip to content

Conversation

@dhellmann
Copy link
Member

@dhellmann dhellmann commented Aug 8, 2025

While working on #695, I noticed that the bootstrapper was changing the dependency type for installation dependencies of build dependencies. That led to invalid graphs, showing for example that torch had no installation dependencies because it was a build dependency of vllm.

Fix that error, and improve logging to show the dependencies discovered and where processing is starting.

@dhellmann dhellmann requested a review from a team as a code owner August 8, 2025 19:55
@dhellmann dhellmann requested a review from tiran August 8, 2025 20:01
@dhellmann dhellmann marked this pull request as draft August 8, 2025 20:38
@dhellmann dhellmann marked this pull request as ready for review August 8, 2025 21:40
@tiran
Copy link
Collaborator

tiran commented Aug 9, 2025

This change reverts my commit f61f142 from 5 months ago. I vaguely remember that the commit addressed a bug, but I cannot recall any details.

Perhaps we need to mark the installation requirements of a build requirement as both install requirement and build-system requirement?

@dhellmann
Copy link
Member Author

This change reverts my commit ref61f142668b4279263b25c4775de9b053a099b03 from 5 months ago. I vaguely remember that the commit addressed a bug, but I cannot recall any details.

That reference isn't working for me, but f61f142 does.

Perhaps we need to mark the installation requirements of a build requirement as both install requirement and build-system requirement?

I do remember that, but don't remember the origin, either. Was there an actual behavior problem, or did it just look like the data was wrong?

Without this change the graph file produced for vllm is incorrect. Torch is both a build dependency and an installation dependency, but since it's treated as a build dependency first every installation dependency of torch shows up as a build dependency and is left out of the install set. That means we can't show the proper install set in a graph visualization, but it also means they are being ignored when we produce the constraints file, which is worse.

When we produce the installation set from a graph, we start at the top-level and traverse all edges that have type "install", so I think we're already treating everything correctly in that point.

@tiran
Copy link
Collaborator

tiran commented Aug 9, 2025

I think it's related to sdist-only mode. I'm not near a computer to verify the hypothesis right now. In sdist-only mode, Fromager does not build wheels unless the package is flagged as build dependency.

@dhellmann
Copy link
Member Author

I think it's related to sdist-only mode. I'm not near a computer to verify the hypothesis right now. In sdist-only mode, Fromager does not build wheels unless the package is flagged as build dependency.

OK, that makes sense. I think we should separate that logic from what we store in the graph. We can look through the current "why" list to see if anything there is a build dependency instead of using only the current edge's type. I'll make that update next week.

The bootstrapper was changing the dependency type for installation
dependencies of build dependencies. That led to invalid graphs, showing
for example that torch had no installation dependencies because it was
a build dependency of vllm.

Fix that error, and improve logging to show the dependencies discovered
and where processing is starting.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@dhellmann dhellmann force-pushed the clarify-dependency-adding branch from 487d8ea to d09f8da Compare August 10, 2025 15:12
@mergify mergify bot added the ci label Aug 10, 2025
@dhellmann dhellmann force-pushed the clarify-dependency-adding branch from d09f8da to dd79c00 Compare August 10, 2025 15:14
A package needs to be built if it is a build dependency or it is an
installation dependency in a chain of packages that lead to a build
dependency.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>
@dhellmann dhellmann force-pushed the clarify-dependency-adding branch from dd79c00 to a92e6c8 Compare August 10, 2025 15:16
@dhellmann
Copy link
Member Author

I think it's related to sdist-only mode. I'm not near a computer to verify the hypothesis right now. In sdist-only mode, Fromager does not build wheels unless the package is flagged as build dependency.

OK, that makes sense. I think we should separate that logic from what we store in the graph. We can look through the current "why" list to see if anything there is a build dependency instead of using only the current edge's type. I'll make that update next week.

I've updated this PR with a new method on the bootstrapper to figure out whether the current context relates to a build dependency. I think I have the logic right, but we may need some more complex test cases to be sure. Let me know what you think, @tiran

Copy link
Collaborator

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Oh, that's a clever approach. I haven't thought of looking through the why list. I like it.

I have one question.

Copy link
Contributor

@rd4398 rd4398 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
I will leave it to @tiran to approve since he suggested changes

@mergify mergify bot merged commit e4ef7ca into python-wheel-build:main Aug 11, 2025
112 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants