googleapis / gapic-generator-python

Generate Python API client libraries from Protocol Buffers.
Apache License 2.0
122 stars 69 forks source link

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

Closed ipotuzhnov closed 2 years ago

ipotuzhnov commented 2 years ago

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

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,
                    ),
                ],
            ),
        ),
    ],
)
  2. Create a `Dockerfile`

FROM python:3 RUN pip install google-cloud-video-transcoder WORKDIR /usr/src/app COPY . .

  3. 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

  4. Observe the error

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" ]


#### 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,
                    }
                ],
            },
        ),
    ],
)
breogangf commented 2 years ago

@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 commented 2 years ago

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

ipotuzhnov commented 2 years ago

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.