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

Inconsistent protobuf versions kclvm->python client #158

Closed
diefans opened this issue Oct 24, 2024 · 5 comments · Fixed by #159
Closed

Inconsistent protobuf versions kclvm->python client #158

diefans opened this issue Oct 24, 2024 · 5 comments · Fixed by #159
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed

Comments

@diefans
Copy link

diefans commented Oct 24, 2024

Bug Report

I stumbled upon an inconsistency of protobuf versions used and also pinned/validated:

In the python client protobuf is pinned to >=4.25.3 (https://github.com/kcl-lang/lib/blob/main/python/pyproject.toml#L18)
The python client protobuf itself validates against 5.27.0 (https://github.com/kcl-lang/lib/blob/main/python/kcl_lib/api/spec_pb2.py#L14)

And the most relevant part is, that the kclvm/api implements also a different version. If you take the example Python client example you receive class references, which are not implemented in protobuf >= 5.27.0:

import kcl_lib.api as api

exec_args = api.ExecProgram_Args(k_filename_list=["api/tests/schema.k"])
args = api.GetSchemaTypeMapping_Args(exec_args=exec_args)
api = api.API()
result = api.get_schema_type_mapping(args)

assert result.schema_type_mapping.__module__ == "google._upb._message"
assert result.schema_type_mapping.__class__.__name__ == "MessageMapContainer"

If you try to import that class directly from protobuf you get an import error:

from google._upb._message import MessageMapContainer
*** ImportError: cannot import name 'MessageMapContainer' from 'google._upb._message' (/home/olli/code/bm/integration-config/.devenv/state/venv/lib/python3.12/site-packages/google/_upb/_message.abi3.so)

There is an open issue protocolbuffers/protobuf#16935 (comment) which describes this.

kcl-lib == 0.10.3

@Peefy
Copy link
Contributor

Peefy commented Oct 24, 2024

Thanks for the feedback. Maybe we need to change the protobuf deps to >=5.27.0 👀

@Peefy Peefy added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed labels Oct 24, 2024
@diefans
Copy link
Author

diefans commented Oct 28, 2024

@Peefy FYI: when I generate the python stub for https://github.com/kcl-lang/lib/blob/main/spec/spec.proto with https://github.com/kcl-lang/rust-protoc-bin-vendored, The generated python file claims to contain protobuf version 4.25.3:

# -*- coding: utf-8 -*-
# Generated by the protocol buffer compiler.  DO NOT EDIT!
# source: spec.proto
# Protobuf Python Version: 4.25.3
"""Generated protocol buffer code."""
from google.protobuf import descriptor as _descriptor
from google.protobuf import descriptor_pool as _descriptor_pool
from google.protobuf import symbol_database as _symbol_database
from google.protobuf.internal import builder as _builder
# @@protoc_insertion_point(imports)

_sym_db = _symbol_database.Default()
# ...

which was also your pinned version before your version bump.

When I use uv pip install protobuf==4.25.3 I can import from google._upb._message import MessageMapContainer and also test if the result.schema_type_mapping is an instance of it:

(Pdb++) root.__class__
<class 'google._upb._message.MessageMapContainer'>
(Pdb++) from google._upb._message import MessageMapContainer
(Pdb++) isinstance(root, MessageMapContainer)
True

...So I think, the protobuf version of _kcl_lib.cpython-312-x86_64-linux-gnu.so is different than the generated python stub and should all be pinned to 4.25.3, since that seems to be the version you use for kclvm. Even though the newer protobuf python versions seem to work, one cannot type check against e.g. MessageMapContainer.

@Peefy
Copy link
Contributor

Peefy commented Oct 28, 2024

I see. Thank you! I seem to have used a higher version of protoc locally instead of KCL rust vendor's protoc, thank you. Additionally, PRs are also welcome, and we can fix them at 4.25.3. 👍

@Peefy
Copy link
Contributor

Peefy commented Nov 4, 2024

Hello @diefans

#161 I've changed the python protobuf version to 4.25.3, could you help review it? Thank you!

@Peefy
Copy link
Contributor

Peefy commented Nov 5, 2024

Closed by #161

@Peefy Peefy closed this as completed Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants