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

FEAT: Ability to set sub labels for specific events #2949

Merged

Conversation

NickM-27
Copy link
Collaborator

@NickM-27 NickM-27 commented Mar 11, 2022

Implementation for #2900

This adds an API to update a specific event with the name or type. For example, I am using Double-Take and can use this to add a sub label so it shows in the UI going from Person -> Person: Nick.

@NickM-27
Copy link
Collaborator Author

Here is an example showing this with the current UI. Top is with sublabel and bottom is without (no change):

Screen Shot 2022-03-11 at 2 41 59 PM

@NickM-27
Copy link
Collaborator Author

@blakeblackshear I will be testing this for at least the next few days (I setup some automations in HASS to set the sub label automatically) and will ping you when it is ready for review, but I wanted to make sure it is something you agree with / think is a good idea. My plan is to submit a PR to double-take as well so it can be automatic without user-input needed, and then be leveraged by other use cases if users want to, but I want to make sure it makes sense.

frigate/http.py Outdated Show resolved Hide resolved
migrations/008_add_sub_label.py Outdated Show resolved Hide resolved
web/src/routes/Events.jsx Outdated Show resolved Hide resolved
@blakeblackshear
Copy link
Owner

I would format the body as json in the docs. No need to specify the header info as it's pretty standard for all clients that would send json anyway.

@NickM-27
Copy link
Collaborator Author

Seeing very weird behavior where the sub_label field disappears after a reboot

@NickM-27
Copy link
Collaborator Author

NickM-27 commented Mar 12, 2022

I am going to undo making sub_label null, it seemed to have a lot of data integrity failures and peewee exceptions after a second restart for some reason, not really sure why but using "" works 🤔

@blakeblackshear
Copy link
Owner

Making a field nullable in the table definition is not the same thing as setting None as a default value for a non-nullable field.

Needs to be like this:

sub_label=pw.CharField(max_length=20, null=True),

https://docs.peewee-orm.com/en/latest/peewee/api.html#fields

@NickM-27
Copy link
Collaborator Author

NickM-27 commented Mar 13, 2022

@blakeblackshear Thanks, I did try that in an earlier commit and I was seeing an error where the migration worked fine but then after another restart the field didn't show up in /api/events and it directly returned (CharField<Unbound>).

Will try again and will see what happens.

@blakeblackshear
Copy link
Owner

Make sure your database is getting reset back to the previous schema between changes.

@NickM-27
Copy link
Collaborator Author

NickM-27 commented Mar 13, 2022

@blakeblackshear I was using rm -rf debug/ after each run to make sure the db started fresh. It seems like default=None in combination with null=True was the issue. Just using null=True, which was your suggestion, I hadn't done and that was likely the issue. Thanks for checking and suggesting that.

frigate/events.py Outdated Show resolved Hide resolved
frigate/events.py Outdated Show resolved Hide resolved
frigate/http.py Outdated Show resolved Hide resolved
frigate/http.py Outdated Show resolved Hide resolved
@NickM-27
Copy link
Collaborator Author

@blakeblackshear I am glad you mentioned in progress events. I made the changes you suggested and I am still seeing events with sub_label set in progress being reset to null when the event ends. Is there anywhere that I need specifically retain the value of sub_label? Since it isn't specifically being set anywhere I figured it wouldn't. I will do some more digging and see if I can pinpoint where it is happening.

@NickM-27
Copy link
Collaborator Author

NickM-27 commented Mar 13, 2022

I am actually seeing this with retain_indefinitely as well, if it is retained while in progress that is reset when the event is saved. I am thinking it is due to the use of Event.replace in the EventProcessor, perhaps I need to get the correct value into event_queue somehow.

@blakeblackshear
Copy link
Owner

I'm sure there is a way to tell peewee to only update some of the fields. By default it probably passes everything. Previously there wasn't anything that could be updated.

@blakeblackshear
Copy link
Owner

I think it needs to be an update call rather than a replace.

@NickM-27
Copy link
Collaborator Author

@blakeblackshear I tried changing it to an update call. The issue I am having trouble solving is that it seems the db entry is created on the first update event. So I am not sure a good way to do the logic if no db entry then insert otherwise update without adding if Event.get(): which would create a lot of db activity. Probably something that peewee has a way of handling as well

@NickM-27
Copy link
Collaborator Author

Okay I think I have an idea on how to do this

@NickM-27
Copy link
Collaborator Author

Okay I think my solution is clean and should work well in all cases 👍

@NickM-27
Copy link
Collaborator Author

After a few days of testing, has been working very well and consistently:

Screen Shot 2022-03-15 at 12 46 28 PM

@NickM-27 NickM-27 changed the title FEAT: Ability to set sub labels for person events FEAT: Ability to set sub labels for specific events Mar 16, 2022
@blakeblackshear blakeblackshear merged commit b1cc64d into blakeblackshear:release-0.11.0 Mar 17, 2022
@NickM-27 NickM-27 deleted the person-sublabels branch March 17, 2022 12:28
@ehn
Copy link
Contributor

ehn commented Mar 18, 2022

Vaguely related to this, it would be neat to have a hierarchy of labels with increasing precision, for example, person/man/nick, vehicle/car, animal/dog. If you could get a match for animal even if the confidence for dog is too low, say, that would be useful.

@interbiznw
Copy link

After a few days of testing, has been working very well and consistently:

Screen Shot 2022-03-15 at 12 46 28 PM

is there any documentation, forum posts, etc on how to integrate this with HA/Deepstack?

@NickM-27
Copy link
Collaborator Author

@interbiznw There is the preview docs: https://deploy-preview-2829--frigate-docs.netlify.app/integrations/api#post-apieventsidsub_label

If you are using doubletake then the recent versions support this automatically. If you are just using HA then you can use the API directly from within HA.

@interbiznw
Copy link

@interbiznw There is the preview docs: https://deploy-preview-2829--frigate-docs.netlify.app/integrations/api#post-apieventsidsub_label

If you are using doubletake then the recent versions support this automatically. If you are just using HA then you can use the API directly from within HA.

Thanks, I am using doubletake but do not see the person name tags automatically? is there something that needs to go in the config?

@interbiznw
Copy link

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.

4 participants