-
Notifications
You must be signed in to change notification settings - Fork 94
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
[FIX] Make Python version explicit in pre-commit workflow #196
[FIX] Make Python version explicit in pre-commit workflow #196
Conversation
Recently (or recently enough that I've just begun noticing it), the nodeenv dependency had troubles installing. Sometimes. It wasn't very reliably failing. This is related to <ekalinin/nodeenv#328>. By explicitly setting the Python version to something <=3.10, we can safely pretend the problem doesn't exist. Ideally the Python versions match up to the ones used in test.yml, but they don't, and I fear it might be too intrusive to fix that in this commit. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
Do you have any explanation on why the workaround works? 🤔 |
Faintest idea. (A part of) the stacktrace in the linked issue:
I have to assume that something goes wrong in In any case, setting an explicit version is Good, Actually. If I had more time than I do, I would research this further and contribute to upstream. |
According to ekalinin/nodeenv#328 (comment) it seems this is a network problem, and it seems the next release for nodeenv will include the patch mentioned there as a workaround and as a fix for the flaky error message. I mean that, AFAICS, probably we'll be still getting the same error every now and then. Or maybe a slightly different one, but still an error. Actually I'm having the same failures on some internal pipelines that still don't use precommix, and all I need is to retry, and see them go green. |
All I know is that coopiteasy/addons#262 (which uses a minorly modified oca-addons-repo-template) consistently failed to merge (but did NOT fail the regular tests on a fastforwardable branch) until I set an explicit Python version. |
That error had appeared to me several times (but a lot percentage), but retrying fixes the problem, so I was considering this a glitch. |
Just now I hit the error in our internal CI:
version information
error information
As you see, it's a network error. Retrying makes it go ✅ . And this CI is running on python 3.9. My recommendation is to set up a cache, so successful runs cache the pre-commit installation and avoid fetching from this buggy endpoint. Or... well, use precommix 😆 #175. Closing because, although the issue is real, the fix doesn't seem to be real AFAICS. Please don't hesitate to continue the conversation or reopen if needed. |
Agreed. Just bad luck on my part regarding the consistent failure. Even disregarding the linked issue, though, I think it would be good to pin the Python version. When Python 3.12 is released (and GitHub's |
Pinging @yajo to review the above comment. |
Recently (or recently enough that I've just begun noticing it), the nodeenv dependency had troubles installing. Sometimes. It wasn't very reliably failing.
This is related to ekalinin/nodeenv#328.
By explicitly setting the Python version to something <=3.10, we can safely pretend the problem doesn't exist.
Ideally the Python versions match up to the ones used in test.yml, but they don't, and I fear it might be too intrusive to fix that in this commit.