Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GenAI: allow configuring additional send trigger after_significant_updates as well as event_end #16919

Merged
merged 18 commits into from
Mar 4, 2025

Conversation

leccelecce
Copy link
Contributor

@leccelecce leccelecce commented Mar 3, 2025

Proposed change

Based on discussion on #16528, a partial implementation to add configuration that allows users to add an extra trigger point on GenAI when a certain number of images have been accumulated. This is disabled by default.

Removed the check for a blank description in the existing end of event code, to ensure the finalized event description is still updated (I'm not sure why this check was necessary; what would have already added a description?).

Draft until my devcontainer starts working again; I will do docs updates too.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code
  • Documentation Update

Additional information

  • This PR fixes or closes issue: fixes #

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • [] The code has been formatted using Ruff (ruff format frigate)

Copy link

netlify bot commented Mar 3, 2025

Deploy Preview for frigate-docs ready!

Name Link
🔨 Latest commit 937920a
🔍 Latest deploy log https://app.netlify.com/sites/frigate-docs/deploys/67c725f050042d0008386ee3
😎 Deploy Preview https://deploy-preview-16919--frigate-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@NickM-27
Copy link
Collaborator

NickM-27 commented Mar 3, 2025

(I'm not sure why this check was necessary; what would have already added a description?).

the user, manually via the UI

@hawkeye217
Copy link
Collaborator

Removed the check for a blank description in the existing end of event code, to ensure the finalized event description is still updated (I'm not sure why this check was necessary; what would have already added a description?).

That check was to prevent a genai description from overwriting a user's entered description if they manually entered something before the end of a long-running event.

@leccelecce
Copy link
Contributor Author

Makes sense. Would you want some additional metadata stored on events to reflect whether a user-input description had been added, and if so to avoid any GenAI calls?

@hawkeye217
Copy link
Collaborator

Makes sense. Would you want some additional metadata stored on events to reflect whether a user-input description had been added, and if so to avoid any GenAI calls?

Not sure we need to be that specific. I think if a user has entered a manual description, GenAI should just not overwrite it. A user can delete their own description and regenerate if they want GenAI for it.

@leccelecce
Copy link
Contributor Author

Not sure we need to be that specific. I think if a user has entered a manual description, GenAI should just not overwrite it. A user can delete their own description and regenerate if they want GenAI for it.

The problem as it stands is that my new "early" GenAI request will populate the description on the event, so the guard upon event finalization for an already-populated description will always fail and it won't run. Unless I rework this to not update the Event entity for an early description generation and embed for semantic search, and instead only publish to MQTT, given most of the use case for this is going to be Home Assistant notifications.

@hawkeye217
Copy link
Collaborator

The problem as it stands is that my new "early" GenAI request will populate the description on the event, so the guard upon event finalization for an already-populated description will always fail and it won't run. Unless I rework this to not update the Event entity for an early description generation and embed for semantic search, and instead only publish to MQTT, given most of the use case for this is going to be Home Assistant notifications.

I think a quick note in the docs would be fine to indicate that if a user has GenAI enabled and they are entering manual descriptions, that their own description may be overwritten depending on their triggers.

@leccelecce
Copy link
Contributor Author

OK, keeping this in draft a while longer. Trying to load the event via Event.get(id) doesn't work as it doesn't seem to have been persisted at the point this runs, so I get frigate.models.EventDoesNotExist. Several downstream methods expect to be passed a full Event, e.g. to enable event fields to be substituted into prompts, so I'd need to refactor those quite a bit.

Any suggestions on preferred approach - assuming I'm not reading this wrong...

@NickM-27
Copy link
Collaborator

NickM-27 commented Mar 3, 2025

need to at a minimum check the has_clip and has_snapshot values of the object to know if it has an event

@leccelecce leccelecce marked this pull request as ready for review March 3, 2025 23:39
@leccelecce
Copy link
Contributor Author

OK, this works. But I need to test it more extensively to see how much fine-tuning it needs. Essentially, I was expecting 2-3 "significant updates" would be enough to send some nice early images from the event. However the timing of when the event is actually saved varies, so the value I've configured might be exceeded by that logic elsewhere in object processing.

It should be fine to merge, a bit of time with it is required to see if it would need to be more fundamentally refactored to not have to wait for an "actual" event.

@leccelecce
Copy link
Contributor Author

As an aside, my testing has been hampered by the debug logging still not working, even in my devcontainer:

logger:
  default: info
  logs:
    frigate.genai: debug
    frigate.embeddings.maintainer: debug

...nada

@leccelecce leccelecce changed the title GenAI: allow configuring "send_after_frames" for early request GenAI: allow configuring additional send trigger after_significant_updates as well as event_end Mar 4, 2025
@NickM-27
Copy link
Collaborator

NickM-27 commented Mar 4, 2025

Have you rebuilt the dev container? I believe Josh was unable to reproduce on dev and I've never had an issue personally

@leccelecce
Copy link
Contributor Author

Have you rebuilt the dev container? I believe Josh was unable to reproduce on dev and I've never had an issue personally

Doesn't work for me in either production or devcontainer. I stop the dev container, docker system prune -a and rebuilt in VS code but not joy.

Now I've got my dev server back online I'll try and have a dig around. Shouldn't be a problem for this PR though as it's an existing issue.

@leccelecce
Copy link
Contributor Author

Raised #16933 to track the debug logging issue, otherwise this PR can be merged when you're happy with it.

@hawkeye217
Copy link
Collaborator

I think once the remaining references to event in the config and docs are updated, this can be merged.

@NickM-27 NickM-27 merged commit c236533 into blakeblackshear:dev Mar 4, 2025
9 checks passed
@leccelecce leccelecce deleted the genai-send-early branch March 4, 2025 17:22
@leccelecce
Copy link
Contributor Author

Thank you both for your support and efforts in reviewing this.

Testing it out for an hour on my street-facing camera, with after_significant_updates: 3 I'm typically getting updates sent a few seconds into a tracked object appearing via the tracked_object_update topic, which is much more useful for real-time notifications than 15-20 seconds until the event completes.

Might be a bit more refinement required based on the variability of when the updates are sent, but will observe for a few weeks first.

I know there is some continuing debate over the use cases for GenAI, I do see it as offering a huge amount of potential for Frigate to get smarter and smarter, and hopefully this is a small step towards that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add trigger for genai submission in addition to end of lifecycle
3 participants