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

fix RestrictedUnpickler #16574

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Conversation

wkpark
Copy link
Contributor

@wkpark wkpark commented Oct 21, 2024

it does no harm to remove the pytorch_lightening dependency in the safe.py (torch.load)

see also comfyanonymous/ComfyUI@735ac4c

Checklist:

it does no harm to remove the pytorch_lightening dependency

see also comfyanonymous/ComfyUI@735ac4c
w-e-w
w-e-w previously requested changes Oct 22, 2024
Copy link
Collaborator

@w-e-w w-e-w left a comment

Choose a reason for hiding this comment

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

it does no harm to remove the pytorch_lightening dependency in the safe.py (torch.load)

NO
we do use those to load LDSR upscaller models

@w-e-w w-e-w closed this Oct 22, 2024
@wkpark
Copy link
Contributor Author

wkpark commented Oct 22, 2024

it does no harm to remove the pytorch_lightening dependency in the safe.py (torch.load)

NO we do use those to load LDSR upscaller models

did you tested? this is a torch.load() fix. without pytorch_lightening in the unpicker, LDSR ckpt model can be loaded without an issue.

@w-e-w
Copy link
Collaborator

w-e-w commented Oct 22, 2024

okay you are right, it dose seems to load
it would seem my assumption about how pickle works is wrong

that being said, I still don't see a point of fixing something that isn't broke
unlike comfy is not like this change will allows to remove pytorch_lightning as it still used elsewhere

reopening this letting others decide

@w-e-w w-e-w reopened this Oct 22, 2024
@w-e-w w-e-w dismissed their stale review October 22, 2024 03:56

reopening this letting others decide

@wkpark
Copy link
Contributor Author

wkpark commented Oct 22, 2024

okay you are right, it dose seems to load it would seem my assumption about how pickle works is wrong

that being said, I still don't see a point of fixing something that isn't broke unlike comfy is not like this change will allows to remove pytorch_lightning as it still used elsewhere

reopening this letting others decide

As you may have guessed, a number of my recent PRs have been related to optimising startup loading time.
some PRs are experimental (can be switched on/off), some PRs have been extracted from my recent Flux1 PR to reduce the total number of commits (not exactly related to the Flux parts)

the import time of pytorch_lightening is about ~5sec on my machine. (almost the same as imports torch)
so Im trying to reduce any dependency of pytorch_ligenting from core.

thank you for your concern!

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