Skip to content

Conversation

@jpg0
Copy link
Contributor

@jpg0 jpg0 commented Jul 2, 2025

Work in progress; not yet ready for merging...

jpg0 and others added 7 commits June 15, 2025 13:23
This will only work if Immich supports it, if not it will fallback to the old method
Also refactors code to disambiguate preloading versus not pools
- Standardized ImmichApi mock instantiation across test suites using constructor with dummy arguments (`new Mock<ImmichApi>("http://dummy-url.com", new HttpClient())`) to resolve proxy creation issues.
- Corrected IApiCache mock instantiation to `new Mock<IApiCache>()`.
- Refined Moq `Verify` calls to use property-based matching (`It.Is<T>(predicate)`) instead of specific object instances for DTOs, fixing verification failures in `AllAssetsRemotePoolTests`.
- Made `BaseCircuitBreaker.DoCall` async-aware to correctly handle exceptions from asynchronous primary operations, allowing the circuit breaker fallback to be properly tested and trigger. Updated consuming classes accordingly.
- Adjusted cache count assertion in `AllAssetsRemotePoolTests.GetAssetCount_CallsApiAndFallsBack` to reflect expected interactions after the circuit breaker fix.
- Updated Moq setup/verify in `PersonAssetsPreloadPoolTests.LoadAssets_CallsSearchAssetsForEachPerson_AndPaginates` to use a detailed predicate helper for `MetadataSearchDto`.

These changes resolve a large number of test failures. One test (`LoadAssets_CallsSearchAssetsForEachPerson_AndPaginates`) remains failing due to a Moq verification mismatch that requires further investigation.
- Corrected Moq predicate in `PersonAssetsPreloadPoolTests` with a detailed helper method `VerifyMetadataSearchDto` including explicit HasValue checks for nullable properties. (Note: This test still fails, Moq verification issue persists).
- Adjusted assertion in `AllAssetsRemotePoolTests.GetAssetCount_CallsApiAndFallsBack` for `_fakeCache.Count` to be 2, as the circuit breaker fix correctly logs fallback and the cache interacts twice.

This resolves all but one test failure. The remaining failure in `PersonAssetsPreloadPoolTests.LoadAssets_CallsSearchAssetsForEachPerson_AndPaginates` is a persistent Moq verification issue.
@jpg0
Copy link
Contributor Author

jpg0 commented Jul 2, 2025

I am opening this PR to discuss the path to removing asset caching.

I have done a little work on this and the current state is this:

  • I was working with the assumption that we would support any version of immich; falling back to preloading assets when query options are not supported
  • Immich has recently added the ability to query by albumId & personId, which unlocks the ability for us to include this when pulling random images (plus applies to the count queries I recently added).
  • The only holes in the Immich API to support all our queries are now:
    • No support to exclude AlbumIds (tbh this is a bit of an odd query; Immich would like to add some kind of exclusions, not sure if it would support our use case)
    • No ability to randomly pull memories

I also just realised that our ShowFavorites flag adds all the favorite photos into the results, rather than filters only favorites, like the Immich API does. We could handle this by creating two pools (all favourites, all that match other filters), although this will result in dupes if the favorites match the queries too (and would give them double the chance of appearing, maybe fine given that they match twice). The other option is to switch to the Immich semantics and filter rather than combine.

So this would mean there would be a main pool of photos, plus possibly a favorites pool if we keep the existing behaviour.

We also support ExcludedAlbums, which Immich doesn't. They do want to though. As this is an exclusion, rather than an inclusion like all the other filters, it's a bit awkward to implement when not caching. Pulling photos is fine (we can skip the ones that are excluded), but counting the total is problematic. The simplest option is to just ignore the excluded counts (this just means that there will marginally more chance of photos from the main pool being selected), a slightly more accurate option may be to subtract the count of all (potentially) excluded assets, which will probably be a bit more accurate but swing the probabilities the other way (note that if we don't have a favorites pool, and the user is not using memories, the probability doesn't matter as there's only a single pool).

The last thing is memories, which needs to be a distinct pool (as Immich don't want these combined into photo search results). I will add a resource to Immich to pull random memories (which I haven't yet done).

One thing that I will point out is that there will probably be quite a bit of extra code when supporting older versions of Immich too - lots of additional pool types, caching options + circuit breakers that fall back from remote calls to caching ones. If you don't think we should go down this path (and just require the latest version), then please let me know and I'll simplify things!

@3rob3
Copy link
Collaborator

3rob3 commented Jul 3, 2025

I don't think it is unreasonable to require a minimum version of Immich server, this is pretty common practice and would simplify things greatly for us. Just put it in the release notes as the first item.
I assume you changed the behavior of Favorites and Memories with your last big change? Previously it would show ONLY memories if it was set (which I found somewhat useless), but I just tested and it doesn't behave that way any more.

@jpg0
Copy link
Contributor Author

jpg0 commented Jul 3, 2025

Well actually I've been a bit confused about how all the options interacted, however, looking at the source that I copied I don't think that I changed anything:

IEnumerable<AssetResponseDto> assets = new List<AssetResponseDto>();
if (_settings.ShowFavorites)
assets = assets.Concat(await GetFavoriteAssets());
if (_settings.ShowMemories)
assets = assets.Concat(await GetMemoryAssets());
if (_settings.Albums.Any())
assets = assets.Concat(await GetAlbumAssets());
if (_settings.People.Any())
assets = assets.Concat(await GetPeopleAssets());

This is the source from before my changes, and you can see that assets are concatenated together, both favorites & memories. I believe that the only option that you can provide that removes images is ExcludedAlbums.

@3rob3
Copy link
Collaborator

3rob3 commented Jul 3, 2025

Sorry, i wasn't very clear. If you ONLY set ShowMemories (no albums, people, or favorites) it would only show you memories. I just did that but it is just showing me random images now.

@jpg0
Copy link
Contributor Author

jpg0 commented Jul 4, 2025

Hmm, I certainly didn't intend to change that, the code is pretty similar:

if (!accountSettings.ShowFavorites && !accountSettings.ShowMemories && !accountSettings.Albums.Any() && !accountSettings.People.Any())
{
return new AllAssetsPool(_apiCache, _immichApi, accountSettings);
}
var pools = new List<IAssetPool>();
if (accountSettings.ShowFavorites)
pools.Add(new FavoriteAssetsPool(_apiCache, _immichApi, accountSettings));
if (accountSettings.ShowMemories)
pools.Add(new MemoryAssetsPool(_apiCache, _immichApi, accountSettings));
if (accountSettings.Albums.Any())
pools.Add(new AlbumAssetsPool(_apiCache, _immichApi, accountSettings));
if (accountSettings.People.Any())
pools.Add(new PersonAssetsPool(_apiCache, _immichApi, accountSettings));
return new MultiAssetPool(pools);

@3rob3
Copy link
Collaborator

3rob3 commented Jul 4, 2025

Can you try? Maybe it’s just me?

@jpg0
Copy link
Contributor Author

jpg0 commented Jul 19, 2025

Immich PR to add random memory fetching: immich-app/immich#20025

@jpg0
Copy link
Contributor Author

jpg0 commented Jul 19, 2025

Can you try? Maybe it’s just me?

Are the random images actually images in memories? Looking at the code it is pulling memories, then aggregating all the assets that form that memory. They were also then being shuffled (fixed in #436), so could just look like random images?

@3rob3
Copy link
Collaborator

3rob3 commented Jul 24, 2025

Can you try? Maybe it’s just me?

Are the random images actually images in memories? Looking at the code it is pulling memories, then aggregating all the assets that form that memory. They were also then being shuffled (fixed in #436), so could just look like random images?

I'm an idiot, sorry. It is working fine. I had settings as ENV in compose, but also had Json settings i forgot about. It of course prefers Json so i wasn't changing anything. ugh.....

@jpg0
Copy link
Contributor Author

jpg0 commented Nov 26, 2025

Update: the recent Immich release included my last change, which was to allow fetching a (limited & random) set of memories. This was the last change to Immich to allow us to purely fetch on-demand, without having to sort or filter ourselves. The only remaining thing to decide ahead of doing this would be how we handle the cases where we've diverged from how Immich perform searching/filtering:

  • ShowFavorites - we union the favorites with the rest, immich intersects them
  • ExcludedAlbums - immich does not support exclusions

Both of these can be worked around, the decision is whether we want to stay close to what Immich provides versus diverge from it (which gives more flexibility for users, but creates more work here).

@JW-CH
Copy link
Collaborator

JW-CH commented Dec 3, 2025

Update: the recent Immich release included my last change, which was to allow fetching a (limited & random) set of memories. This was the last change to Immich to allow us to purely fetch on-demand, without having to sort or filter ourselves. The only remaining thing to decide ahead of doing this would be how we handle the cases where we've diverged from how Immich perform searching/filtering:

  • ShowFavorites - we union the favorites with the rest, immich intersects them
  • ExcludedAlbums - immich does not support exclusions

Both of these can be worked around, the decision is whether we want to stay close to what Immich provides versus diverge from it (which gives more flexibility for users, but creates more work here).

I'd personally would love to ditch the whole logic and let immich do the heavy lifting, but honestly, I don't think this is really an option as you said it would limit the flexibility for users. Do you have something specific in mind?

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.

3 participants