-
Notifications
You must be signed in to change notification settings - Fork 2k
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 Python requirements.txt
file with pinned version numbers
#5474
Comments
FWIW, I only see this suggested by one script, not many - |
@RecRanger Can you show how exactly the script doesn't work with the latest version of protobuf? Can you try and fix that? Thank you! |
I don't see anything wrong with continuing to use the older version of Protobuf (which is tested, validated, etc.). Upgrading the dependency seems like a pointless endeavor for no gain. The error message explicitly says that the current Python code uses features that are deprecated in |
@RecRanger I would still be interested in having such detail recorded in here, as it could be useful when we revisit the issue later. So I'd appreciate it if you show the issue you ran into, perhaps as a piece of copy-paste from the terminal. Thank you! |
Fair enough, here you go!
I'm seeing just now that another potential work-around is to force that environment variable within the Python script. Running this gets past the imports at least: |
Cool. Can you also try embedding the setting of this variable inside the script? Does Maybe you can patch this/these script(s) as needed to use this workaround by default, and then not pin the version of |
I have no strong opinion whether the environment variable approach should be favored over the version constrained. Assuming this really works, it would eliminate the need of the pinned version. On the other hand, I don't know whether support for this workaround will stay for the foreseeable future or if it will eventually break. Using the variable would also hide that the script should ideally be made compatible with latest protobuf. I guess we would at least need a comment in the code explaining why we set the variable. |
Yes, should have that comment. |
I'm still in favor of pinning dependencies, at least as ranges. The user can choose to try with the latest dependencies if they want, but there should be a known working configuration documented in this repo, in the form of a requirements.txt or pyproject.toml |
OK. But if we can also include an easy workaround that makes the script work across a wider range of dependency versions, I think we should. |
It is not clear what version of protobuf is required. Installing the latest version with
pip install protobuf
, as suggested by many of the Python scripts, does not work.Please add a
requirements.txt
file in the root of the repo which pins the versions of Python dependencies.The text was updated successfully, but these errors were encountered: