Add Google Photos service for uploading content #124956
Conversation
home-assistant
bot
added
cla-signed
has-tests
integration: google_photos
new-feature
labels
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
allenporter
force-pushed
the
google_photos_upload
branch
from
fee0aed to
556d54c
Compare
| translation_key="no_access_to_path", | ||
| translation_placeholders={"filename": filename}, | ||
| ) | ||
| if not Path(filename).exists(): |
There was a problem hiding this comment.
I/O inside the event loop. I would move this whole for block inside a single executor job. That way you also don't have to jump between asyncio and executor land if there are multiple files.
| token = await self.async_get_access_token() | ||
| session = aiohttp_client.async_get_clientsession(self._hass) | ||
| try: | ||
| result = await session.post( |
There was a problem hiding this comment.
We want all transport calls to be part of a 3rd party library.
There was a problem hiding this comment.
I was definitely considering if its worth extracting a library for this or not.
Would you expect a library for this single upload call or a wrapper for the whole library including the other google photos api calls? In general the google photos libraries are already pretty thin anyway so thinking it makes sense to move out everything.
There was a problem hiding this comment.
Yeah, the library could handle the async interface that is now in the integration.
There was a problem hiding this comment.
There is https://github.com/omarryhan/aiogoogle Async Google API Client + Async Google Auth. Note I have no experience with it.
There was a problem hiding this comment.
I do think aiogoogle is fine, though still low level given how generic it is, like the google python library. Not bad at all, just still quite low level. I looked at this in the past for calendar:
#70140 (comment)
In that case, i ended up also providing local sync for google calendar. Not sure that we'll want to do that for Photos though.
I started this https://github.com/allenporter/python-google-photos-library-api copying the patterns of the google nest and calendar libraries i maintain and will incorporate your feedback into it.
| client_api = config_entry.runtime_data | ||
| upload_tasks = [] | ||
| file_results = await hass.async_add_executor_job( | ||
| _read_file_contents, hass, call.data[CONF_FILENAME] |
There was a problem hiding this comment.
Should you have multiple tasks like you have for the upload below? Should the reading happen together with the upload?
There was a problem hiding this comment.
An advantage of moving the reading together with the upload, and if you support resumable uploads, is that you could support uploading large files. Right now all the files to be uploaded need to fit in memory before even starting the upload.
There was a problem hiding this comment.
Yeah, I don't love reading all the files at once, and agree read + file upload together makes sense. e.g. https://docs.aiohttp.org/en/stable/client_quickstart.html#streaming-uploads
Having some sort of limit on # of concurrent uploads could be helpful for throttling as well.
Perhaps can handle in the new library also.
| for mime_type, content in file_results: | ||
| upload_tasks.append(client_api.upload_content(content, mime_type)) | ||
| upload_tokens = await asyncio.gather(*upload_tasks) | ||
| media_ids = await client_api.create_media_items(upload_tokens) |
There was a problem hiding this comment.
Do you want to support partial successes? Right now if a single upload fails the code will never call batchCreate. You could call batchCreate for the ones that succeeded but then you would also need to change the API response at line 105 below. Not sure it's worth the complexity.
There was a problem hiding this comment.
I don't want to support partial success given it's an advanced technique for callers to handle. While it would be more efficient, it would mean the API for service responses is more complex and was decided against generally in home-assistant/architecture#777
Maybe could be considered if some sort of on_error handler is implemented
home-assistant/architecture#845 that way the default behavior is fail closed with an option to resume if needed?
An alternative could be to not support multiple files at all in a single call which would also be ok...
| @@ -70,6 +72,40 @@ async def list_media_items( | |||
| ) | |||
| return await self._execute(cmd) | |||
|
|
|||
| async def upload_content(self, content: bytes, mime_type: str) -> str: | |||
There was a problem hiding this comment.
The API also supports resumable uploads which are more reliable and can be retried. It would add a lot of complexity that I'm not sure it's worth it. Maybe something to consider if you extract this to a 3rd party library.
There was a problem hiding this comment.
Yeah, agreed it would be cool, though maybe not needed. I could see this being pretty helpful for larger video use cases. I agree that having the 3rd party library implement this could be the way to go, and maybe it could work like a little backup producer/consumer queue.
Proposed change
Add Google Photos service for uploading content. This adds additonal scopes for uploading content and only enables capabilities based on the authorized scopes.
Milestones for future PRs include:
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: