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

Abort immediately on metadata generation failure instead of backtracking #10722

Merged
merged 3 commits into from
Jan 27, 2022

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Dec 10, 2021

Toward #10655.

@pradyunsg pradyunsg added C: dependency resolution About choosing which dependencies to install type: bugfix C: build logic Stuff related to metadata generation / wheel generation labels Dec 10, 2021
@pradyunsg pradyunsg force-pushed the stop-backtracking-on-build-failures branch from 79d1e5f to deca826 Compare December 10, 2021 16:06
@pradyunsg
Copy link
Member Author

pradyunsg commented Dec 10, 2021

There's a bunch more to do here and this isn't a complete solution to #10655 on its own.

  • Need to improve our error messages on build failures (which I consider a blocker TBH, because the failure output right now sucks; but that's something I'll tackle as part of Add additional diagnostics to our error messages #10421).
  • Should create supporting documentation for this to guide users on how to solve these issues. We learnt this lesson as part of our work on the dependency resolver, that providing documentation that clearly states what the user can do when they see something wierd, is a good thing.
  • Add an opt out flag from the new behaviour.

@pradyunsg pradyunsg force-pushed the stop-backtracking-on-build-failures branch from deca826 to 2905226 Compare December 10, 2021 16:26
@pradyunsg pradyunsg changed the title Abort immediately on build failures instead of backtracking Abort immediately on metadata generation failure instead of backtracking Dec 10, 2021
@notatallshaw
Copy link
Member

notatallshaw commented Dec 11, 2021

When the new resolver was introduced one of the immediate problems was that some packages had their version number wrong and it caused the metadata to be failed to be built and pip would exit, see: #9203

This was particularly problematic for packages with large dependencies or where this happened with transitive dependencies, and at the time all the recommended workarounds failed to remedy the situation: #9203 (comment)

Things may be better now as the ecosystem has had a year to mature. But I don't know if any of the advised workarounds have been fixed, I know adding top level user requirements won't prevent pip from downloading specific package versions, yanking the package to prevent pip from downloading is better but still somewhat depends on #10625 merging, I don't know what the issue with the constraints file was.

So assuming the constraints file issue is fixed I think it's important that the error message to the metadata building failure includes the following information:

  1. The failure is with the package, not with pip, and the user should check that they have the requirements installed for the package
  2. If the user wants to avoid pip checking a specific version of the package they should add an exclusion to the constraints file (with link to the constraints file docs as I think most users will confuse this with the requirements file)
  3. If the user owns the package and the version is problematic they should consider yanking the package (it's clear from some discussions that at least some owners aren't aware of this functionality)

However if it's still possible that pip downloads a specific version even though it's excluded in the constraints file I think there really needs to be an option to override this behavior.

My 2 cents anyway, perhaps all of this is already done in the better error reporting and I'm just repeating things you are already aware of 🙂

@pradyunsg
Copy link
Member Author

This still backtracks if a package has inconsistent metadata, which was the problem in #9203.

It doesn't, however, backtrack when a project fails to compile or generate metadata.

@notatallshaw
Copy link
Member

This still backtracks if a package has inconsistent metadata, which was the problem in #9203.

It doesn't, however, backtrack when a project fails to compile or generate metadata.

That makes sense, but I think my comments still stand.

Imagining the hypothetical scenario PopularTransativeDependencyPackage v2 was released as source only and didn't compile easily on Windows but PopularTransativeDependencyPackage v1 was still mostly used by Windows users and included a compiled wheel for Windows. This PR would affect that scenario right?

@pradyunsg
Copy link
Member Author

pradyunsg commented Dec 12, 2021

Yes. Now, let's tweak that case, imagine instead of just v2, it goes all the way through to v35 (all like the v2 you described). What is a good behaviour here? Now imagine even v1 doesn't work on Windows. What is a good behaviour here?

The question is not whether the behaviour changes, but rather if it is better to fail eagerly instead of trying really hard to get an answer.

At this point I'm repeating OP from #10655, so... I'm going to stop.

@pradyunsg pradyunsg closed this Dec 12, 2021
@pradyunsg pradyunsg reopened this Dec 12, 2021
@notatallshaw
Copy link
Member

notatallshaw commented Dec 12, 2021

Yes. Now, let's tweak that case, imagine instead of just v2, it goes all the way through to v35 (all like the v2 you described). What is a good behaviour here? Now imagine even v1 doesn't work on Windows. What is a good behaviour here?

The question is not whether the behaviour changes, but rather if it is better to fail eagerly instead of trying really hard to get an answer.

At this point I'm repeating OP from #10655, so... I'm going to stop.

Well in that scenario the install eventually fails. Where as in the scenario I outlined where adding to constraints fails to block downloading a specific version then there is a valid solution to install but it's impossible to use pip to install that valid solution.

Therefore if the issue of constraints still exists then the good behavior is categorically to try all versions so not to have impossible to install scenarios, or at least to have an option to keep that behavior.

However if the constraints issue has been solved then I agree fast failing is correct, but the user should have a clear error message (as I outlined above) so they understand they can add to the constraints if they need to.

@pradyunsg
Copy link
Member Author

However if the constraints issue has been solved then I agree fast failing is correct

Cool, added a test to make sure that this indeed the case.

@pradyunsg
Copy link
Member Author

pradyunsg commented Dec 12, 2021

Well in that scenario the install eventually fails.

Sure. If it hasn't filled up your disk trying to build those packages. If it hasn't been killed due to being out of memory on a memory-constrainted VM/Docker container. If it hasn't hit a timeout in your CI / documentation build, or hit the limit of a human's patience.

Then, yes, you will get a failure from pip after failing to build every release of $package, because it doesn't support the platform you're running on or you're missing a required build-time dependency -- something you could tell from the first build failure and some Googling.

Anyway, your example is literally a repeat of something that's already been discussed in #10655. Let's move the discussion there, since none of your concerns are about the actual PR implementation, but whether the behaviour changes are reasonable.

@pradyunsg pradyunsg force-pushed the stop-backtracking-on-build-failures branch from 7236358 to f35b53e Compare December 12, 2021 13:08
@pradyunsg pradyunsg force-pushed the stop-backtracking-on-build-failures branch from f35b53e to e5125fd Compare January 21, 2022 16:12
@pradyunsg pradyunsg force-pushed the stop-backtracking-on-build-failures branch 4 times, most recently from a21ae3b to 4cd5fd2 Compare January 25, 2022 08:23
@pradyunsg pradyunsg added this to the 22.0 milestone Jan 26, 2022
@pradyunsg
Copy link
Member Author

I'm leaning toward deferring the documentation updates to be a part of #10421 and #9475's error messages page.

That's a much bigger effort, and I'd like to defer it to deal with this in that.

@pradyunsg pradyunsg mentioned this pull request Jan 26, 2022
@pfmoore
Copy link
Member

pfmoore commented Jan 26, 2022

I'm happy to defer the docs, as long as they don't get forgotten. Maybe we could have an explicit issue for the docs as a reminder, but simply link it to #10421 and #9475?

@pradyunsg
Copy link
Member Author

I'll leave #10655 open. :)

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Jan 27, 2022
This behaviour is more forgiving when a source distribution cannot be
installed (eg: due to missing build dependencies or platform
incompatibility) and favours early eager failures instead of trying to
ensure that a package is installed regardless of the amount of effort it
takes.
This serves as an opt-out from build failures causing the entire
installation to abort.
@pradyunsg pradyunsg force-pushed the stop-backtracking-on-build-failures branch from 4cd5fd2 to 9d0db88 Compare January 27, 2022 18:13
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jan 27, 2022
@pradyunsg pradyunsg merged commit 98b1022 into pypa:main Jan 27, 2022
@pradyunsg pradyunsg deleted the stop-backtracking-on-build-failures branch January 27, 2022 20:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: build logic Stuff related to metadata generation / wheel generation C: dependency resolution About choosing which dependencies to install
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abort immediately when there's a build failure, instead of backtracking
4 participants