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

Generated client libraries don't handle proto messages with "mapping" field properly #1113

Closed
ipotuzhnov opened this issue Dec 14, 2021 · 3 comments · Fixed by #1114
Closed
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@ipotuzhnov
Copy link

TL;DR

Transcoder API uses a field named mapping in proto message TextStream which appears to be used in a base class for a proto message, this makes it difficult to use the client library with Transcoder API.
I've noticed that for a somewhat similar case (field type) the autogenerated code added _ to the end of the name of the field making it type_, shouldn't this be done for fields named mapping as well?


One of the Transcoder API customers reported that they were having difficulties using Python client library for Transcoder API to create a job to generate subtitles (TextStream).

Environment details

  • Programming language: python
  • OS:
  • Language runtime version: Python 3.9.6 (default, Aug 17 2021, 15:37:33)
  • Package version: pip list
    google-api-core 2.0.0
    google-auth 2.0.0
    google-cloud-video-transcoder 0.5.0
    googleapis-common-protos 1.53.0
    grpcio 1.39.0

Steps to reproduce

  1. Save the following code snippet in a file mapping_issue.py
from google.cloud.video import transcoder_v1

if __name__ == "__main__":
    job.config = transcoder_v1.types.JobConfig(
        elementary_streams=[
            transcoder_v1.types.ElementaryStream(
                key="vtt-stream-en",
                text_stream=transcoder_v1.types.TextStream(
                    codec="webvtt",
                    # The following doesn't work because it looks like that "mapping"
                    # is a "reserved" argument name in GCP python client libraries
                    # https://github.com/googleapis/proto-plus-python/blob/main/proto/message.py#L447
                    mapping=[
                        transcoder_v1.types.TextStream.TextMapping(
                            atom_key="atom0",
                            input_key="srtEN",
                            input_track=0,
                        ),
                    ],
                ),
            ),
        ],
    )
  1. Create a Dockerfile
FROM python:3
RUN pip install google-cloud-video-transcoder
WORKDIR /usr/src/app
COPY . .
  1. Build a docker image and run the code snippet in it
docker build . -t p3 && docker run -it --rm -v ~/.config/gcloud:/root/.config/gcloud p3 python ./mapping_issue.py
  1. Observe the error
Traceback (most recent call last):
  File "/usr/src/app/./mapping_issue.py", line 8, in <module>
    text_stream=transcoder_v1.types.TextStream(
  File "/usr/local/lib/python3.9/site-packages/proto/message.py", line 494, in __init__
    raise TypeError(
TypeError: Invalid constructor input for TextStream: [atom_key: "atom0"
input_key: "srtEN"
]

Workaround

Use python dictionary

from google.cloud.video import transcoder_v1

if __name__ == "__main__":
    job.config = transcoder_v1.types.JobConfig(
        elementary_streams=[
            transcoder_v1.types.ElementaryStream(
                key="vtt-stream-en",
                text_stream={
                    "codec": "webvtt",
                    # Use python dictionary as a workaround
                    "mapping": [
                        {
                            "atom_key": "atom0",
                            "input_key": "srtEN",
                            "input_track": 0,
                        }
                    ],
                },
            ),
        ],
    )
@ipotuzhnov ipotuzhnov added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Dec 14, 2021
@breogangf
Copy link

@ipotuzhnov There's no documentation nor examples on how to generate multiple language subtitles/captions so this makes implementing such a thing very difficult.

The observed error can, for sure be improved, as the information provided is not very useful:

Traceback (most recent call last):
File "/usr/src/app/./mapping_issue.py", line 8, in
text_stream=transcoder_v1.types.TextStream(
File "/usr/local/lib/python3.9/site-packages/proto/message.py", line 494, in init
raise TypeError(
TypeError: Invalid constructor input for TextStream: [atom_key: "atom0"
input_key: "srtEN"
]

While facing this error, we used a python dictionary, as there's the only way we found to generate a JobConfig Object using TextStream mappings, it was very time consuming, by try an error approach.

@software-dov
Copy link
Contributor

This is an unfortunate limitation of the Proto Plus library: mapping is a special optional parameter used in the constructor of proto.Message, which can't be changed for backwards compatibility reasons.
The workaround is to add mapping to the list of reserved names which are disambiguated when generating message classes: instead of mapping we would get mapping_. I know this is less than desirable, but it's the least bad option available.
The other reserved words are either python keywords, e.g. class, from, etc. or builtins, e.g. list, any, set. Keywords must be disambiguated due to limitations in the python parser, and we need certain builtins unclobbered for use in the generated surface.

PR adding mapping to the reserved words is here: #1114

@software-dov software-dov linked a pull request Dec 15, 2021 that will close this issue
@ipotuzhnov
Copy link
Author

Hi Dov, thank you for a quick fix! I hope this will land soon, so we can add examples to our client library for Python.

@breogangf I understand your frustration. The API is very flexible and we can't cover all use cases, but we continuously work on improving our documentation and planning to add samples for CC/subtitle usage as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants