-
Notifications
You must be signed in to change notification settings - Fork 556
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
Bumping deps to an intermediary step compatible with Python3.11 #4005
Conversation
e7e91a0
to
22e1108
Compare
src/platform_requirements.txt
Outdated
@@ -1,2 +1,16 @@ | |||
grpcio==1.38.0 | |||
grpcio==1.62.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right that we need to add so many platform_requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are transitive dependencies which are C extensions, and were previously not declared.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah.
So I don't always love the idea of pinning dependencies because it can confuse what we are actually interested in and makes it harder to upgrade the main package. Can we just leave these out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather declare every single C dependency. Otherwise pipenv lock will choose those for us, and I have no assurance that whatever it chooses will be compatible with python 3.11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most of these are defined in the pipfile and you can just define them there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Everything in the root Pipfile (https://github.com/google/clusterfuzz/blob/master/Pipfile) is expected to be installed on the system. This will certainly require C-extensions (and possibly transitively).
src/Pipfile (which is separate from the root Pipfile) contains the packages that we download and re-package into zips for deployment. In theory, only grpcio and protobuf should require C extensions there, because we've never had any issue on Mac/Windows with just patching in the platform-specific versions of these two (which is what platform_requirements.txt does).
Eventually, we do want to move everything from the root Pipfile into platform_requirements.txt, but perhaps we can do this more iteratively. e.g. just move a minimal seet that we have to absolutely upgrade for Python 3.11 support for now (e.g. psutil?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and perhaps with PYTHONPATH/sys.path ordering, we don't even have to uninstall the existing version of psutil on the system if the newer one we provide in the source zip takes precedence over the system one.
see
def fix_module_search_paths(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm there is a >0 chance that @jonathanmetzman #3663 was actually correct, and Windows only broke because it was not running psutil >= 5.9.4. The largest version I saw in our windows init scripts was 5.7.0, which is not compatible with py 3.11.
I might be wrong, but this seems very plausible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, i thought this was your theory. I thought i succeeded in upgrading the version on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was working under the assumption that the update succeeded on linux but not on windows . Is that the case?
… path on http 302 redirections - https://datatracker.ietf.org/doc/html/rfc7231#section-7.1.2
7aa17be
to
1a39670
Compare
Some of our current dependencies are incompatible with Python 3.11. This PR attempts to bump the least possible amount of dependencies (more specifically, grpcio, grpcio-tools, psutil and markupsafe), so we can isolate deployment issues between dependency bumps and the python upgrade itself.
*** Motivation This PR complements #4005 by bumping the gcp clients, grpcio and protobuf, and fixing the code to accomodate for breaking changes.
Motivation
Some of our current dependencies are incompatible with Python 3.11. This PR attempts to bump the least possible amount of dependencies (more specifically, grpcio, grpcio-tools, psutil and markupsafe), so we can isolate deployment issues between dependency bumps and the python upgrade itself.
This PR belongs to this effort.