Convert files to data URLs on webhook post #2185
Conversation
8W9aG
commented
Mar 6, 2025
8W9aG
commented
- When posting a webhook we can end up with files referencing /tmp folders that can’t be accessed by the consumer of the webhook
- Convert these files to data URLs so they can be consumed.
* When posting a webhook we can end up with files referencing /tmp folders that can’t be accessed by the consumer of the webhook * Convert these files to data URLs so they can be consumed.
* upload_files needs to know if the payload has a Path object
python/cog/server/webhook.py
Outdated
| if "output" in response_object: | ||
| response_object["output"] = upload_files( | ||
| response_object["output"], | ||
| upload_file=upload_file, # type: ignore | ||
| ) |
There was a problem hiding this comment.
What was the thinking of doing this here in the webhook handler rather than in the worker where we currently do the file uploads?
cog/python/cog/server/runner.py
Lines 452 to 453 in 018aca5
There was a problem hiding this comment.
Doing it there breaks a lot of tests, assumed that this was required behaviour based on those tests, and given that the problem is targeted on webhooks it made sense to move it to there. Let me know if you think any of that is mistaken.
| "input": {}, | ||
| } | ||
| response = PredictionResponse(**payload) | ||
| payload["output"] = "data:None;base64,aGVsbG8gd29ybGQ=" |
There was a problem hiding this comment.
Is None supposed to be here?
| payload["output"] = "data:None;base64,aGVsbG8gd29ybGQ=" | |
| payload["output"] = "data:text/plain;base64,aGVsbG8gd29ybGQ=" |
There was a problem hiding this comment.
Actually, what is this test doing?
There was a problem hiding this comment.
It tests that the webhook is sent data URLs rather than tmp file directories. I can change the mime-type however it seems odd to me that the upload_file function uses None as a default if that default is considered itself invalid. Can you comment on that?
| "output": Path(tmpfile.name), | ||
| "input": {}, | ||
| } | ||
| response = PredictionResponse(**payload) |
There was a problem hiding this comment.
Ok just for my understanding, without the change we would've passed back the path to the /tmp file which then the caller could not access, so we're uploading the file (somewhere) and passing a URL back? And then the line below has a b64 encoded URL
