Skip to content

Conversation

@yunsaki
Copy link
Contributor

@yunsaki yunsaki commented Aug 26, 2022

Looking at the dream.py I noticed that files tend to stay open for a long time. This is a bad practice with potential impact on system resources. If an open file is not handled correctly and an exception occurs the file will never be closed. This is why in my opinion the use of a context manager when dealing with files is a must in python. Furthermore I would argue that in a context manager you should only do what's absolutely necessary and then exit.

This is what I'm trying to do with this small merge request.

Please test if this still functions as intended.

@yunsaki
Copy link
Contributor Author

yunsaki commented Aug 26, 2022

I would also like to note that the function write_log_message still takes t2i as an argument, which it doesn't seem to utilise.

(also I kinda lied with the commit message in 0f28663. if var is also False if the value of the variable is considered as empty; "", [] and {} for example wouldn't be True.)

@yunsaki
Copy link
Contributor Author

yunsaki commented Aug 26, 2022

Couldn't confirm if it works with GFPGAN. Speaking of which, is it intended that every after every generated image this warning comes up: "GFPGAN not initialized, it must be loaded via the --gfpgan argument"?

@Oceanswave
Copy link
Contributor

No - sorry for that, fix is in main now, PR #101

@lstein
Copy link
Collaborator

lstein commented Aug 27, 2022

Thanks for picking up these problems. This is a bad habit of mine from Perl programming, where filehandles are closed automatically as soon as they go out of scope.Will do some testing tomorrow morning and then merge.

@lstein lstein self-assigned this Aug 27, 2022
@yunsaki
Copy link
Contributor Author

yunsaki commented Aug 27, 2022

Using context managers in general should be pretty safe, but I would still advice to only spend as much time as necessary in them, at least in the case of file handling.

@lstein lstein self-requested a review August 28, 2022 16:26
@lstein
Copy link
Collaborator

lstein commented Aug 28, 2022

@yunsaki The handling of --from_file by turning it into a list of lines almost works but not quite. The issue is that some users have requested the ability to read from standard input so that they can use the script in a pipeline or with named pipes. I tried this, and found that the current code has to wait until the EOF before it does the "\n" split. In the case of a pipeline this will cause deadlock. I think that it's actually OK not to create a file context for the from_file in this particular case because the script is going to end as soon as the EOF happens anyway.

I've merged the other part of your commits and I'm reverting the from_file handling. If you have a better solution let me know.

@yunsaki
Copy link
Contributor Author

yunsaki commented Aug 28, 2022

Alright, fair enough! I was the most unsure about that anyway. Might do some testing myself, but for now I don't have a better solution.

@lstein lstein merged commit 8b9a520 into invoke-ai:main Aug 28, 2022
@yunsaki
Copy link
Contributor Author

yunsaki commented Aug 28, 2022

@lstein Regarding refactoring; should I also implement the morphing dream main loop for the main branch? I think it has a few advantages, mostly in the sense that you have less indentations and don't need to do stuff like done = True; break but just return True. You could also argue that that is less readable, because looking at the code you need always need to know that return True means "stop the loop" and return False "continue the loop".

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.

3 participants