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 timeout to pypi version request, preventing undue hangups on startup #855

Merged
merged 3 commits into from
Dec 13, 2024

Conversation

ntjess
Copy link
Contributor

@ntjess ntjess commented Nov 8, 2024

Fixes #853

@maartenbreddels
Copy link
Contributor

We thought a bit about it. What do you think of this solution? Where we only show it if we found a new version on time.

@ntjess
Copy link
Contributor Author

ntjess commented Nov 12, 2024

Have you tested the timing between the browser opening and the message being printed in the console?

My only concern with this new approach is the following scenario:

  • User runs solara run ...
  • Browser window opens, user no longer thinks about their terminal
  • Upgrade message prints, but user never sees it because their focus is already on the browser
  • App generates print() / logging statements that obfuscate the upgrade message
  • User never learns there is a solara update

@ntjess
Copy link
Contributor Author

ntjess commented Nov 12, 2024

Another thought: We may also want to read the pip conf (through an approach like pip config list -> check if index-url is overwritten) to see which URL should be hit instead of defaulting to pypi.

This is a more robust solution than just a timeout, and would work for e.g. private pypi setups like artifactory. However, it is nontrivial to know ahead of time where pip will fetch from (overloads at global/user level, even per-pip command). So maybe the complexity outweighs the benefits.

@maartenbreddels
Copy link
Contributor

I revered to your original idea, but we now execute it in a thread, so it is never delaying on the terminal. We still add a timeout, so the thread doesn't hang around (500ms should be enough, overwise indeed, focus is probably shifted).

Using a Lock/mutex, we make sure the printing of text does not mix.

@maartenbreddels maartenbreddels merged commit acfb977 into widgetti:master Dec 13, 2024
27 checks passed
@maartenbreddels
Copy link
Contributor

Thanks @ntjess !

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.

Add a short timeout to _check_version() requests call
2 participants