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

Improve initialization for extra arches #281

Merged
merged 4 commits into from
Jul 11, 2024

Conversation

akx
Copy link
Contributor

@akx akx commented Jul 4, 2024

This PR is related to AUTOMATIC1111/stable-diffusion-webui#16144, where we have to do some contortions to install extra arches when Spandrel is late-imported.

It

  1. makes the ValueError for duplicate architectures a specific exception type, so it can be caught...
  2. ... by the new spandrel_extra_arches.install() convenience function, which will return False (and not raise) in case the main registry already has (any of) the arches it tries to wire up.

@akx akx force-pushed the spandrel-extra-arches-init branch from b06397d to 94b44c5 Compare July 4, 2024 06:48
@akx akx marked this pull request as ready for review July 4, 2024 06:49
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

There are 2 things I want to talk about:

  1. I really like the install function, but I think its API is a little weird. It only returns False for a single type of failure and throws otherwise. This also contradicts the documentation of the function.

    I think a better API would be def install(*, ignore_duplicates=False) -> None. Basically, it throws by default, but you can use install(ignore_duplicates=True) to ensure that the second install is a noop.

    Implementation-wise, we just have to add a ignore_duplicates=False argument to ArchRegistry#add, and then install can be implemented as simply:

    def install(*, ignore_duplicates: bool = False) -> None:
        """TODO: Add docs"""
        MAIN_REGISTRY.add(*EXTRA_REGISTRY, ignore_duplicates=ignore_duplicates)
  2. Throwing a DuplicateArchitectureError is a breaking change. I don't think it's a bad change, but this is something we have to consider, especially since the need for a unique error has been diminished with the ignore_duplicates parameter. Edit: my bad

@akx
Copy link
Contributor Author

akx commented Jul 10, 2024

Throwing a DuplicateArchitectureError is a breaking change.

It's not, IMO, unless you're catching things in a particularly weird way (like, if type(exc) is ValueError weird). Unless I'm missing something?

We used to throw ValueError, now we throw DuplicateArchitectureError which derives from ValueError, so if you have a except ValueError, the new exception would still be caught too.

I'll get back to the other concerns later, work-work is pressing.

@RunDevelopment
Copy link
Member

It's not [...]. We used to throw ValueError, now we throw DuplicateArchitectureError which derives from ValueError

You're absolutely correct. I overlooked that. Please ignore point 2.

@akx akx force-pushed the spandrel-extra-arches-init branch from 94b44c5 to e7e4126 Compare July 11, 2024 05:46
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Just 2 minor nits.

tests/util.py Outdated Show resolved Hide resolved
@akx akx force-pushed the spandrel-extra-arches-init branch from a91ab5e to 11c86dc Compare July 11, 2024 10:27
Copy link
Member

@RunDevelopment RunDevelopment left a comment

Choose a reason for hiding this comment

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

Thank you @akx!

@RunDevelopment RunDevelopment merged commit c3aa562 into chaiNNer-org:main Jul 11, 2024
8 checks passed
@akx akx deleted the spandrel-extra-arches-init branch July 11, 2024 14:00
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.

2 participants