Conversation
Found while trying to bring the package up from a clean checkout: every module
that touches a proto failed with "Descriptors cannot be created directly". The
checked-in *_pb2.py files were produced by a protoc from the Python 2 era
(they still carry the `_b = sys.version_info[0] < 3 and ...` shim) and have no
runtime version check, so protobuf >= 3.19 refuses to load them.
Regenerating from the checked-in .proto files did not work either. Those
import "utils/vector.proto", which makes protoc emit
from utils import vector_pb2 as utils_dot_vector__pb2
whereas the checked-in files use the package-absolute form
from ffn.utils import vector_pb2 as utils_dot_vector__pb2
so the sources and the generated files disagree about the include root, and no
single include root reproduces what is checked in. Prefixing the imports with
ffn/ and generating from the repository root makes the two agree, and emits
the runtime version guard so a future protobuf release fails loudly instead of
confusingly.
Regenerated with:
python -m grpc_tools.protoc -I. --python_out=. \
ffn/utils/vector.proto ffn/utils/bounding_box.proto \
ffn/inference/inference.proto ffn/inference/consensus.proto \
ffn/inference/resegmentation.proto
ffn/utils/vector.proto is unchanged: it has no imports to rewrite. The Apache
header of each generated file was restored afterwards, as protoc replaces it
with its own banner.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While getting the package importable so I could follow the inference path, I found the
checked-in protobuf bindings do not load. On master all five generated modules are
unimportable, for two different reasons:
The first three were generated by a protoc from the Python 2 era: they still carry the
_b = sys.version_info[0] < 3 and ...shim and contain no_runtime_version.ValidateProtobufRuntimeVersioncall, so protobuf >= 3.19 refuses toload them.
The other two fail before they get that far, and would fail on any protobuf version,
because their generated imports are not package-relative:
Those only resolve when
ffn/itself is onsys.path, not when the package isimported as
ffn.*. Note that the two files that do it right use the other form, sothe checked-in generated files are not even consistent with each other.
Regenerating from the checked-in sources does not fix this on its own, because the
sources and the generated files disagree about the include root. The
.protofiles sayso generating with
-I ffnfollows them literally and reproduces the unimportablefrom utils import ...form. Prefixing those imports withffn/and generating fromthe repository root makes the sources and the generated output agree, and the output
then carries the runtime version guard, so a future protobuf release fails with a clear
message instead of the "Descriptors cannot be created directly" one.
Regenerated with:
ffn/utils/vector.protohas no imports of its own, so it is unchanged. The Apacheheader of each generated file was restored afterwards, since protoc replaces it with
its own banner.
Verification
Clean trees exported from
masterand from this branch, same interpreter(protobuf 7.36.2):
One thing to decide
These were generated with protoc 7.35.1, which is what I had to hand. The guard
requires the gencode and the runtime to share a major version, so taking this would
require protobuf 7.x at runtime. If you would rather not move the floor that far, say
the word and I will regenerate with an older protoc (4.x or 5.x); nothing else in the
diff would change.