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

Convert latents_ubyte to 8-bit unsigned int before converting to CPU #6300

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shenanigansd
Copy link

Hello, not sure if this is the best solution, but looks like it fixes the issue described in #3539 for most people, me included, so I'll go ahead and submit this PR.
Fixes #3539

@traugdor
Copy link

traugdor commented Jan 6, 2025

it indeed resolves #3539. This has been an on-going issue for AMD device users and needs to be addressed ASAP.

@comfyanonymous
Copy link
Owner

can you make this only apply on directml?

@Kosinkadink
Copy link
Collaborator

A code snippet to help make the directml check: in comfy.model_management.py, there is a variable called directml_enabled. As the name implies, if it's True, directml was enabled on startup.

import comfy.model_management

if comfy.model_management.directml_enabled:
    # do directml_enabled thing...

@traugdor
Copy link

traugdor commented Jan 7, 2025

I was actually going to suggest this in the morning after sleeping on it, but I see you beat me to it.

Copy link
Author

@shenanigansd shenanigansd left a comment

Choose a reason for hiding this comment

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

Sorry, I'm new here and I don't understand. I'm happy to update the PR, but I'm not understanding where to insert the provided snippet. Is it just this? (plus the import)

latent_preview.py Outdated Show resolved Hide resolved
@Kosinkadink
Copy link
Collaborator

@shenanigansd If you're stuck making that change, I can make a contribution to your PR so that we can get this change through.

@shenanigansd
Copy link
Author

Sorry, I just got busy at work and haven't had a chance to circle back yet. I think I've made the requested changes correctly now in 79c526f. If I messed up, please go ahead and make the correct updates @Kosinkadink.

@Kosinkadink Kosinkadink self-requested a review January 10, 2025 22:15
@Kosinkadink
Copy link
Collaborator

No worries, just wanted to make sure this PR doesn't go stale given a lot of AMD peeps may be impacted by this. The change looks good to me, I don't have an AMD gpu at the moment to test it on, but assuming this works on your machine, I think this can get merged in very soon.

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.

Cannot handle this data type: (1, 1, 3), <f4
5 participants