Add generic binding support #409
Conversation
|
Please open a separate issue to add support to dataclasses with the details |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def main(msg: str) -> str: |
There was a problem hiding this comment.
please add another test without annotation. The purpose of explicity specifying dataType in function.json is to to binding to basic types without annotations
There was a problem hiding this comment.
I believe this should no longer be applicable since we are removing the dependency on data type?
There was a problem hiding this comment.
We do need to have tests for both i.e dataType is specified and not specified.
| @@ -60,6 +60,10 @@ def check_input_type_annotation( | |||
| cls, pytype: type, datatype: protos.BindingInfo.DataType) -> bool: | |||
| if datatype is protos.BindingInfo.undefined: | |||
| return issubclass(pytype, azf_abc.QueueMessage) | |||
| elif (datatype is protos.BindingInfo.binary): | |||
| return issubclass(pytype, bytes) | |||
There was a problem hiding this comment.
what about other bindings? shouldn't they be updated to handle dataType binary and string? what about outputbindings?
There was a problem hiding this comment.
Output bindings already support elementary types, where applicable.
what about other bindings?
Which bindings do you have in mind? Is there actually value in providing raw input for all bindings given that rich bindings already exist? I'm thinking an abundance of options here would create confusion without actual benefit.
There was a problem hiding this comment.
Goal of this work item is to enable all bindings to work when datatype is specified in function.json
This means that, when issue #135 is addressed, users should be able to use functions without adding reference to azure.functions to bind to basic types.
This work-item is also about designing the worker to be a layer between host and any library types users to choose to request in their functions
There was a problem hiding this comment.
This still seems to me like a step backwards. I understand why raw interface might be useful for when there is no canonical azure.functions binding yet, essentially as a workaround.
But our original direction with the design of the worker was to expose well-documented Pythonic interfaces as the API, not raw, untyped, data. Promoting the latter creates a strong binding to the internals, and that may restrict our ability to upgrade existing users to newer versions of the platform.
There was a problem hiding this comment.
Using azure.functions is still the recommended approach. All the documentation and the samples will be using types in azure.functions package.
Again, this work item is to make python worker an interface layer that converts to requested types.
|
@elprans can you add tests here for Azure Table Storage and SignalR bindings?
These are two of the scenarios we don't provide rich bindings for and want to make sure they're tracked and tested with each release. |
|
With these changes, i think both table and signalr bindings should work. @elprans - Please test E2E. |
|
@asavaritayal @pragnagopa I'll open a separate issue for SignalR and Table Storage. This PR already has tests for arbitrary bindings. |
|
I understand setup needed for testing signalR is difficult. Can you please add E2E test for table storage? It just needs storage account connection string which E2E tests are already using. I can signoff once table storage is tested. |
|
@pragnagopa How do I add Table API to an existing Cosmos DB account? |
|
Here is the documentation for table storage bindings: https://docs.microsoft.com/en-us/azure/azure-functions/functions-bindings-storage-table for https://azure.microsoft.com/en-us/services/storage/tables/ . Please note these are not related to cosmosdb |
So is this the wrong doc? https://docs.microsoft.com/en-us/azure/cosmos-db/tutorial-develop-table-dotnet |
|
That's correct. The doc you referred to is for CosmosDB Table API. The test I am referring to is just for Azure Table Storage. Which is similar to Blob, Queue bindings we have already. |
|
@pragnagopa I get this failure: function.json: {
"scriptFile": "__init__.py",
"disabled": false,
"bindings": [
{
"type": "httpTrigger",
"direction": "in",
"name": "req"
},
{
"direction": "in",
"type": "table",
"name": "docs",
"dataType": "string",
"partitionKey": "test",
"rowKey": "foo",
"tableName": "test",
"connection": "AzureWebJobsStorage"
},
{
"type": "http",
"direction": "out",
"name": "$return"
}
]
} |
|
looks like specifying |
|
@pragnagopa Isn't the point here to test unstructured binding via 'dataType'? |
Goal of E2E testing
For 1, open an issue on the host. Parsing table input data as string is failing. I think worker needs to handle any binding that is not defined in azure.functions package as a generic binding without enforcing the use of 'dataType` in function.json |
I don't think this is a good idea. The Rich/Unstructured switch has to be explicit. Otherwise, if we choose to support a binding in a future release, user code will break. |
Can you explain? Generic binding is only fall back if there are no rich types found in any of the referenced assemblies |
|
I cannot write a decent E2E test if only half of the host functionality is working. How do I test that the output binding is working correctly without being able to read the data back? |
You do not have to read data to write data to an output binding. Let's take one step at a time. Does writing using generic output binding work? |
The function doesn't crash, but I have no way to verify if the data was actually written. |
|
can you give example python function for non-text blob? |
I upload a PNG image or a ZIP file using an HTTP trigger, and store that as a blob. |
|
Not sure I understand the issue you are pointing. When using a generic binding, uploading a PNG image or a ZIP file should still work. User needs to set the data on the output binding. Here is the expanded sample of java script function that uploads a local file const fs = require('fs');
const util = require('util');
const readFileAsync = util.promisify(fs.readFile);
module.exports = async function (context, myBlob) {
context.log("JavaScript blob trigger function processed blob \n Name:", context.bindingData.name, "\n Blob Size:", myBlob.length, "Bytes");
context.bindings.myOutputBlob = context.bindings.myInputBlob;
context.bindings.myOutputBlobString = "stringout";
var text = '{ "employees" : [' +
'{ "firstName":"John" , "lastName":"Doe" },' +
'{ "firstName":"Anna" , "lastName":"Smith" },' +
'{ "firstName":"Peter" , "lastName":"Jones" } ]}';
context.bindings.myOutputBlobJson = text;
var data = await readFileAsync('./BlobTrigger1/JavaVersion.png');
context.log(data.length);
context.bindings.myOutputBlobPng = data;
};corresponding function.json It would be best if you can share python function code and function.json for cases you think might not work. |
|
OK, so in generic binding mode the function must return either a |
As long as return type matches supported types by the TypedData defined in protobuf, it should work
Yes. If user specified annotation, we should raise error if return type does not match annotation type. |
|
Expanding on the earlier question:
Type checking only matters when using |
|
Ping! |
|
@elprans - Ping! Please give an update here or over email. |
Per Azure/azure-functions-python-worker#409, the rich binding implementations are now in the library.
|
|
||
| __all__ = ( | ||
| 'Out', 'Context', | ||
| 'is_binding', 'is_trigger_binding', | ||
| 'is_trigger_binding', |
There was a problem hiding this comment.
why do you need 'is_trigger_binding'?
There was a problem hiding this comment.
It is used to distinguish regular input bindings from trigger bindings, because there are differences in handling these (metadata, etc.)
| "name": "msg", | ||
| "queueName": "testqueue-untyped-return", | ||
| "connection": "AzureWebJobsStorage", | ||
| "dataType": "string" |
pragnagopa
changed the title
| @@ -54,6 +54,18 @@ def test_queue_message_object_return(self): | |||
| self.assertEqual(r.status_code, 200) | |||
| self.assertEqual(r.text, 'test-message-object-return') | |||
|
|
|||
| def test_queue_untyped_return(self): | |||
There was a problem hiding this comment.
Not fully understanding the flow of this test case. The put_queue_untyped_return function adds a message to the queue testqueue-untyped-return via an http post. After the sleep, the get_queue_untyped_blob_return reads from an input blob and returns an http response. The second function call does not use the queue message from the first function - is that intended? The naming and structure of the function suggests a relation between the two.
There was a problem hiding this comment.
Each test is independent to avoid races. The flow in each trigger test is "put data" -> "wait for trigger" (trigger writes a blob) -> "check the blob to ensure trigger ran".
There was a problem hiding this comment.
I'm not able to match up the code with flow that you've described. Here's what I see as the flow for each of these requests.
put_queue_untyped_return: POST Http data -> return queue message with bytes type. This is meant to check that we are able to write queue message withbytes?get_queue_untyped_blob_return: GET request -> read blob data -> return data as string via HTTP. The function does not make use of queues so I'm not sure why queue is mentioned in the name. The intention here is to check that HTTP messages can be returned withstrtype?
I'm also not sure why both of these calls are in the same test function test_queue_untyped_return even though they are completely independent and are not using the same resources either.
There was a problem hiding this comment.
The function does not make use of queues so I'm not sure why queue is mentioned in the name. The intention here is to check that HTTP messages can be returned with str type?
The intent is to verify that the queue trigger has fired. There is no way to do this other than make the trigger produce a verifiable side-effect. Writing blobs seems like the most reliable way. The name of the function contains queue because it belongs logically to the same test.
Per Azure/azure-functions-python-worker#409, the rich binding implementations are now in the library.
elprans
force-pushed
the
default-binding
branch
2 times, most recently
from
99b03f3 to
b774843
Compare
Per discussion, the worker no longer includes support for rich bindings by default. The implementation have been moved to azure-functions-python-library, and the runtime dependency on is has been removed.
|
@elprans, should be good to check in? |
Add a generic binding implementation to allow unknown bindings with
dataType: stringanddataType: binary.Fixes: #66
Fixes: #408