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.

Sign up for GitHub

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

Jump to bottom

Better exception handling around self-check #12865

Merged
merged 3 commits into from
Jul 22, 2024
Merged

Better exception handling around self-check #12865

merged 3 commits into from
Jul 22, 2024

Conversation

pradyunsg
Copy link
Member

See #12864 for context.

@pradyunsg
Move exception suppression to cover more of self-version-check logic
be21d82 
This correctly suppresses issues around building networking sessions for
a self version check.
@pradyunsg pradyunsg added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jul 21, 2024
@pradyunsg
Rework how --debug is handled in main
3518d32 
Move the `run` call into a wrapper function and inline the exception
handling while moving the try-finally around `run` into the
nested function.

This reduces the amount of code at a deeper nesting level and moves the
self-version-check logic into the "around the run" exception catching so
that a consistent message is presented in cases of failures.
@pradyunsg pradyunsg force-pushed the better-exception-handling-around-selfcheck branch from 8fa3a0b to 3518d32 Compare July 21, 2024 09:25
@pradyunsg
Properly mock _self_version_check_logic
e503141 
Providing a value return value ensures that downstream code does not try
to pass around invalid values.
@pradyunsg
Copy link
Member Author

Bah, CI failure is the retry tests failing. /cc @ichard26 for awareness.

@ichard26
Copy link
Member

ichard26 commented Jul 21, 2024 via email

@ichard26
Copy link
Member

Looking at the test, it seems to be a flaky failure. I'm going to rekick CI to get this unblocked, but I think these tests may have to go as I'm not sure how to avoid random failures due to system time resolution variation and system CPU load. I think what happened here is that the GHA runner was loaded running other jobs, causing the test to pause execution during the first call for long enough that the second retry is never scheduled as more than 10ms have passed. I would've thought earlier that 10ms ought to be enough for a single Python function call to finish, but I forgot to consider system load.

@ichard26 ichard26 closed this Jul 21, 2024
@ichard26 ichard26 reopened this Jul 21, 2024
@ichard26 ichard26 mentioned this pull request Jul 21, 2024
Open
@pradyunsg pradyunsg added this to the 24.2 milestone Jul 21, 2024
@pradyunsg pradyunsg mentioned this pull request Jul 21, 2024
Closed
@ichard26
Copy link
Member

I can also confirm that this also gracefully handles the self version check error described in #12864.

~/dev/oss/pip ❯ pip install . -q

~/dev/oss/pip ❯ pip install -e . -q
WARNING: There was an error checking the latest version of pip.

@pradyunsg pradyunsg merged commit 203780b into pypa:main Jul 22, 2024
40 of 53 checks passed
@pradyunsg pradyunsg deleted the better-exception-handling-around-selfcheck branch July 28, 2024 22:01
@notatallshaw notatallshaw mentioned this pull request Aug 3, 2024
Closed
1 task
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ichard26 ichard26 ichard26 approved these changes

Assignees
No one assigned
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Milestone
24.2
Development

Successfully merging this pull request may close these issues.
2 participants
@pradyunsg @ichard26