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

Remove legacy temp dir implementation #12332

Closed
wants to merge 4 commits into from

Conversation

catboxanon
Copy link
Collaborator

@catboxanon catboxanon commented Aug 5, 2023

Description

Closes #3278 #11040 #11185

A lot of this code is legacy and no longer needed, as Gradio has fixed all these items. This would also resolve the long standing issue of the temp folder never being cleared, which is now doable since all Gradio related files are stored separately under gradio now.

Some related info:
#3278 (comment)
https://github.com/gradio-app/gradio/blob/34f6b22efbfedfa569d452f3f99ed2e6593e3c21/gradio/components/base.py#L310-L316
gradio-app/gradio#3523
gradio-app/gradio#3597
gradio-app/gradio#4256

This would also prevent any edge cases like this being encountered in the future when Gradio updates. Mikubill/sd-webui-controlnet#1563

This is however a bit of a breaking change in it's current state, as it makes the temp_dir setting obsolete. It should be preferred to use the GRADIO_TEMP_DIR environment variable now. I'm open for suggestions on how to make this more easily accessible to the user though if this is desired.

Checklist:

@catboxanon catboxanon changed the title Cleanup legacy temp dir items Remove legacy temp dir implementation Aug 5, 2023
@catboxanon catboxanon linked an issue Aug 5, 2023 that may be closed by this pull request
1 task
@catboxanon
Copy link
Collaborator Author

catboxanon commented Aug 5, 2023

One more thing I'm not sure about is if we want to default clean_temp_dir_at_start to True now, or just create a separate option entirely (since the old implementation explicitly states it only clears the non-default dir). Setting the current option to true also wouldn't work for many users since at the moment that defaults to false and we don't want to modify any user configurations like that.
My reasoning is I would think for most users they would want the directory cleared regularly (read: on startup), and they would be a bit surprised to find out if it's currently not being cleared (as many have been already).

@AUTOMATIC1111
Copy link
Owner

AUTOMATIC1111 commented Aug 5, 2023

There's no way this is right. The code does more than just write to a user specified temp dir. It also serves image files with correct filenames, and also prevents gradio from making a temporary file at all if the image is already saved.

@catboxanon
Copy link
Collaborator Author

catboxanon commented Aug 5, 2023

You're correct I overlooked the filenames, sorry. I still think this should be resolved somehow to be more consistent with how Gradio handles this now because it consistently crops up for users. It's also preventing the ability for the default temp dir to be cleared, which has now been doable for a long time since Gradio doesn't just dump everything into the root temp dir now.

I'm going to leave it up to someone else to clean this up though as it seems I'm not too familiar with the side effects of this part of the code.

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.

[Bug]: Images left in a temp folder C:\Users\username\AppData\Local\Temp
2 participants