googleapis / python-aiplatform

A Python SDK for Vertex AI, a fully managed, end-to-end platform for data science and machine learning.
Apache License 2.0
615 stars 330 forks source link

function_call.args type MapComposite instead of Struct #3483

Open anuraaga opened 5 months ago

anuraaga commented 5 months ago

Thanks for stopping by to let us know something could be better!

PLEASE READ: If you have a support contract with Google, please create an issue in the support console instead of filing on GitHub. This will ensure a timely response.

Please run down the following list and make sure you've tried the usual "quick fixes":

If you are still having issues, please be sure to include as much information as possible:

Environment details

Steps to reproduce

  1. Invoke a text prompt with a function call
  2. Try to serialize the function call args to JSON using protobuf MessageToJson

This seems like it should work since the library uses a Struct type

https://github.com/googleapis/python-aiplatform/blob/e33d11fa02eb721a6fe09bbd7c2e6a9954dbfe98/google/cloud/aiplatform_v1beta1/types/tool.py#L164

But the actual type seems to be MapComposite, which is not compatible with protobuf. I found MapComposite here

https://github.com/googleapis/proto-plus-python/blob/main/proto/marshal/collections/maps.py

but am not sure why it is the final type. I can use dict(args) to convert to a python dict, but this is only obvious from debugging the runtime types. I guess the type should be correct, either in the definition, or in the runtime behavior.

Code example

response = await self._model.generate_content_async(
      contents,
      generation_config={"temperature": _DEFAULT_TEMPERATURE},
      tools=[tool],
  )
function_call = response.candidates[0].content.parts[0].function_call
args_str = MessageToJson(function_call.args)

Stack trace

  File "/Users/anuraag/git/jarvis/prompt/server/.venv/lib/python3.11/site-packages/google/protobuf/json_format.py", line 174, in ToJsonString
    js = self._MessageToJsonObject(message)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/anuraag/git/jarvis/prompt/server/.venv/lib/python3.11/site-packages/google/protobuf/json_format.py", line 180, in _MessageToJsonObject
    message_descriptor = message.DESCRIPTOR
                         ^^^^^^^^^^^^^^^^^^

Making sure to follow these steps will guarantee the quickest resolution possible.

Thanks!

Ark-kun commented 5 months ago

Try to serialize the function call args to JSON using protobuf MessageToJson

Please use part.to_dict() for serialization.

But the actual type seems to be MapComposite, which is not compatible with protobuf. I found MapComposite here https://github.com/googleapis/proto-plus-python/blob/main/proto/marshal/collections/maps.py but am not sure why it is the final type.

In Python protobuf libraries, Struct is special and is often mapped to dict. And proto representation of a dict/map is MapComposite. The issue lies with the protobuf libraries...

I'll try to make a change so that FunctionCall is a custom non-GAPIC class.

Ark-kun commented 5 months ago

Try to serialize the function call args to JSON using protobuf MessageToJson

MessageToJson works on raw protobuf library messages, not the proto-plus library messages. So you likely need MessageToJson(proto_plus_msg._pb). But that won't work for proto maps. You can serialize the outer message that's not a map (e.g. FunctionCall itself).

anuraaga commented 5 months ago

Please use part.to_dict() for serialization.

I only want to serialize the args, which I think is natural given the nature of function calling. The LLM returns args that need to be sent to a different function.

Serializing something different from args is also an ok workaround but doing a type cast and calling dict works fine too.

Basically the field is explicitly set to : struct_pb2.Struct as a type annotation but the runtime type is different. The current runtime type is fine but I guess then the type annotation needs to be fixed to be accurate.

Basically one question was whether the runtime type was intended to be Struct? It sounds like the answer is no, which is fine, then the only problem is in type checking, not runtime behavior.

vp-code commented 4 months ago

Wasted half an hour before I found this issue. part.to_dict() works, but parsing FunctionCall itself is a very natural thing to do. Upvoting the ask.

meteatamel commented 2 months ago

I also want to upvote this issue. Not clear to me right now, how I can access deserialize the args of FunctionCall

Ark-kun commented 2 months ago

hello. Can you please explain your use case? Are you all really sure you actually want the Struct object instead of a Python dict?

Just to remind, a simple JSON object like this

{
    "param1": "value1",
    "param2": {"key2": [1, 2]}
}

will have the following structure when using Struct:

{
    "fields": {
        "param1": {
            "string_value": "value1"
        },
        "param2": {
            "struct_value": {
                "fields": {
                    "key2": {
                        "list_value": [1, 2]
                    }
                }
            }
        }
    }
}

https://github.com/protocolbuffers/protobuf/blob/fba8024963b8884cfa0d3903b3b0cbed27587f02/src/google/protobuf/struct.proto#L62

How are you planning to actually call a function (the main purpose of FunctionCall) using Struct? Most functions in the world do not really understand proto messages and Structs and cannot parse nested structures with "fields" and "string_value" items.

To me it seems that it's much easier to call a function using a dict:

functions[function_call.name](**function_call.args)
Ark-kun commented 2 months ago

/cc @parthea

anuraaga commented 2 months ago

I think for the most part, people will want a dict. The biggest problem is the type annotation not matching the runtime type, making it difficult to find how to actually get the dict. If a mapping type like MapComposite is intended, that should be the type annotation. If the value was actually a Struct, we could also just use MessageToDict. Either is fine as long as the type is correct or people will continue to trip up on this.