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(server): conditionally run facial recognition nightly #11080

Merged
merged 6 commits into from
Jul 14, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Jul 14, 2024

Description

The facial recognition nightly currently runs every night, but there's no need when no new face has been detected since the last run.

This PR stores the last run date in the system metadata table and compares it with the newest facesRecognizedAt value in the job status table. It only does this in the nightly; manual runs still behave like before.

The query for getting the latest face detection date is a full table scan, but it isn't worth indexing the column just for one non-interactive query a day. It at least avoids sorting.

Resolves #11078

@mertalev mertalev force-pushed the feat/smarter-facial-recognition-nightly branch from aad1512 to 497c689 Compare July 14, 2024 18:08
Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

server/src/services/person.service.ts Outdated Show resolved Hide resolved
@mertalev mertalev enabled auto-merge (squash) July 14, 2024 22:48
@mertalev mertalev merged commit 8193416 into main Jul 14, 2024
22 checks passed
@mertalev mertalev deleted the feat/smarter-facial-recognition-nightly branch July 14, 2024 22:53
this.repository.getLatestFaceDate(),
]);

if (state?.lastRun && latestFaceDate && state.lastRun > latestFaceDate) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a string compare? Does that even work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, since they're both ISO strings.

Copy link
Contributor Author

@mertalev mertalev Jul 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested by hardcoding nightly: true. The first time it ran (since there was no system metadata entry) and the second time it skipped.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Maybe we should add a comment or something. I think it isn't very obvious, which makes it an easy place to introduce a bug.

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

Successfully merging this pull request may close these issues.

3 participants