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

Add more context for Job errors. #2479

Merged
merged 3 commits into from
Jul 28, 2024
Merged

Add more context for Job errors. #2479

merged 3 commits into from
Jul 28, 2024

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Jul 27, 2024

This makes several error paths that manifest when creating locks and
PEXes present better error details.

Several changes support this:

  1. Jobs can now have a context label which is used, when set, to prefix
    all job error output. This should help root cause the problem; i.e.:
    is it fundamentally Pex or is it Pip or is it a problem with a bad
    package?
  2. When a Pip download errors, we now show all STDERR output instead of
    just the last line whenever the last line contains "See above for
    details", which Pip emits to indicate an error buried deeper in the
    logs.
  3. A new log analyzer is added to all Pip downloads that knows how to
    spot sdist build errors and surface their details.
  4. A fail-safe is added to the Pip download log scraper to just report
    the full Pip log content when no better analysis was found.

Fixes #2113

This makes several error paths that manifest when creating locks and
PEXes present better error details.

Several changes support this:
1. Jobs can now have a context label which is used, when set, to prefix
   all job error output. This should help root cause the problem; i.e.:
   is it fundamentally Pex or is it Pip or is it a problem with a bad
   package?
2. When a Pip download errors, we now show all STDERR output instead of
   just the last line whenever the last line contains "See above for
   details", which Pip emits to indicate an error buried deeper in the
   logs.
3. A new log analyzer is added to all Pip downloads that knows how to
   spot sdist build errors and surface their details.
4. A fail-safe is added to the Pip download log scraper to just report
   the full Pip log content when no better analysis was found.

Fixes pex-tool#2113
@jsirois jsirois marked this pull request as ready for review July 28, 2024 19:42
@jsirois jsirois changed the title Add Job context for errors. Add more context for Job errors. Jul 28, 2024
@@ -211,7 +210,7 @@ def bdist(self):
# type: () -> str
get_pip(
interpreter=self._interpreter,
resolver=ConfiguredResolver(pip_configuration=PipConfiguration()),
resolver=ConfiguredResolver.default(),
Copy link
Member Author

Choose a reason for hiding this comment

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

There are several ConfiguredResolver.default() changes like this in the tests below. This makes those test more robust to combinations not run in CI; I hit a few when testing out this change.

@@ -137,6 +137,9 @@ commands =

[testenv:{typecheck,check}]
deps =
# We keep this compatible with Python 2.7 for the 2.7 type check.
Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise unrelated, but helps with dev env setup.

@@ -1566,45 +1565,6 @@ def __repr__(self):
)


def spawn_python_job(
Copy link
Member Author

Choose a reason for hiding this comment

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

This was only used in a test - now inlined.

ResolverVersion.default() is ResolverVersion.PIP_2020,
reason=(
"The PIP_2020 resolver triggers download analysis in normal resolves but this test is "
"concerned with the case when there is no analysis to be performed."
Copy link
Member Author

Choose a reason for hiding this comment

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

There is now always Pip download log analysis; so the test was obsolete.

Copy link
Collaborator

@huonw huonw left a comment

Choose a reason for hiding this comment

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

This looks great. Thanks for improving diagnostics!

@jsirois jsirois merged commit e0aac4f into pex-tool:main Jul 28, 2024
26 checks passed
@jsirois jsirois deleted the issues/2113 branch July 28, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"error downloading required artifacts" is misleading when the artifact is already in the cache
3 participants