Skip to content

feat(mm): support customizable cache key for load_model_from_path() #7987

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

psychedelicious
Copy link
Collaborator

Summary

When loading/caching a model, we use its file path as the cache key.

Some models are initialized with parameters, so the path is not a unique identifier for the model. For these models, the cache key needs to have some other unique data in it.

This PR adds a cache_key_extra: str | None = None arg to the internal model manager load_model_from_path() method. It also adds the arg to the public invocation context methods that wrap the internal method.

If provided, the string is appended to the model's path as its cache key.

Alternatives to this approach include:

  1. Instead of a string, accept any Hashable. Use the hashable as the cache key when provided. Or create a new object that includes the model path and extra hashable, and use that as the key. This approach may require deeper changes, because there may be something that relies on the cache keys being strings.
  2. Use the change in this PR for the internal method, but make the public API take a hashable of model parameters instead of a string. In the public wrapper methods, we would stringify the hashable and pass it on to the internal methods.
  3. Use some fancy introspection to divine the cache key. @JPPhoto shared a draft on discord.

I'm sure there are other strategies that work.

Out of the 4 approaches (this PR + the alternatives), I like Alternative 1 the most. However, I haven't reviewed the cache logic thoroughly yet and am not confident that changing the key to a hashable will not break something. It'll probably be fine - maybe require minor changes - just I haven't investigated yet.

Related Issues / Discussions

https://discord.com/channels/1020123559063990373/1361450510812450907

QA Instructions

There are no functional changes to existing code paths/nodes. This is purely in support of community nodes that want to load models and take advantage of the model manager's caching.

Merge Plan

n/a

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)
  • Updated What's New copy (if doing a release after this PR)

@github-actions github-actions bot added python PRs that change python files services PRs that change app services labels May 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python PRs that change python files services PRs that change app services
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant