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

Allow k_dpmpp_2s_ancestral and k_dpmpp_2m #1767

Closed
wants to merge 1 commit into from

Conversation

spezialspezial
Copy link
Contributor

Tested and works so far for 'k_dpmpp_2s_ancestral' and 'k_dpmpp_2m'. NOT working for 'k_dpm_fast', 'k_dpm_adaptive' out of the box

Tested and works so far for 'k_dpmpp_2s_ancestral' and 'k_dpmpp_2m'. NOT working for 'k_dpm_fast', 'k_dpm_adaptive' out of the box
@hipsterusername
Copy link
Member

@spezialspezial - Any chance you can walk through the purpose of these changes?

It doesn't seem to add any samplers to the constants the app uses, so it doesn't seem to be a full addition of samplers?

@damian0815 damian0815 self-requested a review December 3, 2022 22:37
Copy link
Contributor

@damian0815 damian0815 left a comment

Choose a reason for hiding this comment

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

not approved. this kind of code is a security risk. plus there's no frontend support. compare the amount of code in #1389 - this is what's needed to add a new sampler.

@spezialspezial
Copy link
Contributor Author

@spezialspezial - Any chance you can walk through the purpose of these changes?

The purpose is beeing able to use k_dpmpp_2s_ancestral and k_dpmpp_2m as an api user until you catch up with the webgui.

It doesn't seem to add any samplers to the constants the app uses, so it doesn't seem to be a full addition of samplers?

I'm using prompt2image, the argument is untyped but likely considered a string or None. Not sure about constants. You mean there is an enumeration to use?

I agree, it's a probably not suitable from your point of view. Well, maybe it's useful to know I'm using it for a while now and there seems to be no problem at runtime.

@spezialspezial
Copy link
Contributor Author

Very good samplers by the way, you're missing out

@hipsterusername
Copy link
Member

Would love to have them added, but like @damian0815 said, we'd need you to make the full updates required to support this - Reference #1389 for guidance.

Thanks!

@spezialspezial
Copy link
Contributor Author

Makes sense. I've read about the Mac issues and I understand Apple user support is a concern. I'm not using a Mac nor the Birch-san fork which makes it hard to evaluate. I'm sure some solution will pop up from the Mac base. It's easy to patch for people inclined so no harm.

@damian0815
Copy link
Contributor

Very good samplers by the way, you're missing out

if you scroll up a couple of lines from your changes you'll find they're already implemented

       elif self.sampler_name == 'k_dpmpp_2_a':
            self.sampler = KSampler(self.model, 'dpmpp_2s_ancestral', device=self.device)
        elif self.sampler_name == 'k_dpmpp_2':
            self.sampler = KSampler(self.model, 'dpmpp_2m', device=self.device)

@lstein
Copy link
Collaborator

lstein commented Dec 5, 2022

As far as i'm aware we already support these samplers on all platforms.

@lstein lstein closed this Dec 5, 2022
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.

4 participants