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(web): deduplication UI #9540

Merged
merged 45 commits into from
May 23, 2024
Merged

Conversation

alextran1502
Copy link
Contributor

@alextran1502 alextran1502 commented May 16, 2024

This PR implements

  • UI for deduplication view
  • Endpoint for resolving duplication

Utility Menu

image

Duplication UI - 1

image

Duplication UI - 2

image

Duplication UI - 3

image

Duplication UI - 4

image

Duplication Detection Settings

image

Copy link

cloudflare-pages bot commented May 16, 2024

Deploying immich with  Cloudflare Pages  Cloudflare Pages

Latest commit: e607958
Status: ✅  Deploy successful!
Preview URL: https://9623f74a.immich.pages.dev
Branch Preview URL: https://feat-web-duplication-detecti.immich.pages.dev

View logs

@NicholasFlamy
Copy link
Contributor

Backend for this UI (commenting so that the reference is here):
#8228

@NicholasFlamy
Copy link
Contributor

NicholasFlamy commented May 17, 2024

I tested a fix to this problem:
https://discord.com/channels/979116623879368755/1237891913059930142/1241068695401267230

It's about Duplicate Detection not disabling when Machine Learning is enabled but Smart Search is disabled. Since Duplicate Detection relies on CLIP embeddings it should probably be disabled when Smart Search is disabled.

Should I PR this branch or main? This change is not specific to the UI, it's one small change to the server.

@NicholasFlamy
Copy link
Contributor

I have a version for main and a version for this PR's branch but it applies to the existing implementation so it should probably be added to main:
https://github.com/NicholasFlamy/immich/tree/dupe-det-disable-main-branch

@alextran1502
Copy link
Contributor Author

I tested a fix to this problem: https://discord.com/channels/979116623879368755/1237891913059930142/1241068695401267230

It's about Duplicate Detection not disabling when Machine Learning is enabled but Smart Search is disabled. Since Duplicate Detection relies on CLIP embeddings it should probably be disabled when Smart Search is disabled.

Should I PR this branch or main? This change is not specific to the UI, it's one small change to the server.

@NicholasFlamy Please open a PR to main

@NicholasFlamy
Copy link
Contributor

The change in #9565 should be drop in and shouldn't conflict with anything in this PR branch.

@NicholasFlamy
Copy link
Contributor

If this works across libraries it'd be could to display which library the photo is from (so if they have one photo in the upload library and one in an external library, it should show that.

@alextran1502
Copy link
Contributor Author

@NicholasFlamy this will work with per account, not across instances.

@NicholasFlamy
Copy link
Contributor

NicholasFlamy commented May 18, 2024

@NicholasFlamy this will work with per account, not across instances.

Sorry I didn't phrase it well. I confirmed with Mert that pictures from User A's external library can show up as duplicates of pictures in User A's upload library on the same instance of immich.

My suggestion is when User A goes to the duplicates page, the picture should have a label that tells whether the picture is in User A's external library or User A's upload library.

@alextran1502
Copy link
Contributor Author

@NicholasFlamy we will have to rework the permission/query then. Let move our discussions back to Discord since the pr discussion should be used for code review and code related

@NicholasFlamy
Copy link
Contributor

Should the Duplicates page be hidden or grayed out or something on the Utilities page when Duplicate Detection is disabled? Or, (if it's not too complicated) when there are no Duplicates, gray out the Duplicates page (could even retain the button's functionality, just that it's grayed out).

Comment on lines +100 to +101
step="0.01"
min={0.001}
Copy link
Member

Choose a reason for hiding this comment

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

@alextran1502 you want to stick with 0.01 for now?

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Noice

@@ -6,6 +7,11 @@ export class DuplicateResponseDto {
assets!: AssetResponseDto[];
}

export class ResolveDuplicatesDto {
@IsNotEmpty()
assetIds!: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need @ValidateUUID({ each: true }) here

Comment on lines 105 to 107
!config.machineLearning.enabled ||
!config.machineLearning.clip.enabled ||
!config.machineLearning.duplicateDetection.enabled}
Copy link
Contributor

Choose a reason for hiding this comment

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

might be able to just use a feature flag here instead. $featureFlag.duplicateDetection

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NicholasFlamy What was the reason you didn't use the feature flag? I remember you have a PR put in for this, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added the feature flag to the original backend PR. I didn't think to use the feature flag here (mainly I forgot about this part when I added that flag). So I think a feature flag could replace all of that.

Suggested change
!config.machineLearning.enabled ||
!config.machineLearning.clip.enabled ||
!config.machineLearning.duplicateDetection.enabled}
$featureFlag.duplicateDetection

I haven't tested this, either I'll make the change on my test instance or if it looks good you could commit it and I'll test it, but I won't get to testing either option until tomorrow.

{/key}
{:else}
<div class="text-center text-lg dark:text-white flex place-items-center place-content-center">
No duplication was found on the instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
No duplication was found on the instance
No duplicates were found.

Copy link
Contributor

Choose a reason for hiding this comment

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

And probably a <p> tag instead of a div.

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.

Great work! :)

@alextran1502 alextran1502 merged commit 57d94bc into main May 23, 2024
24 checks passed
@alextran1502 alextran1502 deleted the feat/web-duplication-detection-ui branch May 23, 2024 17:57
!config.machineLearning.enabled ||
!config.machineLearning.clip.enabled ||
!config.machineLearning.duplicateDetection.enabled}
disabled={disabled || $featureFlags.duplicateDetection}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be !$featureFlags.duplicateDetection?

Copy link
Member

Choose a reason for hiding this comment

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

Most certainly

@rithask
Copy link

rithask commented May 24, 2024

an undo option and an option to skip checking duplicate videos would be nice

@mertalev
Copy link
Contributor

It could be nice QoL for the notification to have an Undo button (also for archiving, trashing, etc.). The app could just wait for a certain time before sending the requests and cancel if you click Undo before the notification goes away. But I think this isn't strictly a duplicate-related request.

When you mention videos, I just realized that the duplicate detection will consider an image and a video with a similar thumbnail as duplicates. That doesn't make sense: it should check for assets of the same type.

There is also a possible issue where the thumbnail for videos can be black and spuriously match against other videos. This is a general issue with video thumbnails that we should fix.

But assuming those issues are addressed, would you still like to disable detection for videos? If so, could you elaborate on why?

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.

None yet

7 participants