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

Some minor code tweaks #4303

Closed
wants to merge 18 commits into from
Closed

Conversation

jslegers
Copy link

global folder_names_and_paths is missing in get_folder_paths

"global folder_names_and_paths" is missing in get_folder_paths
@jslegers
Copy link
Author

I also cleaned up some helper functions that either weren't used at all or are pretty useless.

@jslegers jslegers changed the title Missing global keyword Some minor code tweaks Aug 11, 2024
folder_paths.py Outdated
@@ -139,6 +139,7 @@ def add_model_folder_path(folder_name: str, full_folder_path: str) -> None:
folder_names_and_paths[folder_name] = ([full_folder_path], set())

def get_folder_paths(folder_name: str) -> list[str]:
global folder_names_and_paths
Copy link
Contributor

Choose a reason for hiding this comment

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

@mcmonkey4eva mcmonkey4eva added User Support A user needs help with something, probably not a bug. and removed User Support A user needs help with something, probably not a bug. labels Sep 12, 2024
comfy/gligen.py Outdated
@@ -34,7 +21,8 @@ class FeedForward(nn.Module):
def __init__(self, dim, dim_out=None, mult=4, glu=False, dropout=0.):
super().__init__()
inner_dim = int(dim * mult)
dim_out = default(dim_out, dim)
if not dim_out:
dim_out = dim() if isfunction(dim) else dim
Copy link
Contributor

Choose a reason for hiding this comment

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

dim is definitely not a function here

@mcmonkey4eva mcmonkey4eva added the Good PR This PR looks good to go, it needs comfy's final review. label Sep 16, 2024
@mcmonkey4eva
Copy link
Contributor

oh, right, I can just edit PRs to make minor adjustments, nice

@mcmonkey4eva mcmonkey4eva added the Run-CI-Test This is an administrative label to tell the CI to run full automatic testing on this PR now. label Sep 16, 2024
Copy link

(Automated Bot Message) CI Tests are running, you can view the results at https://ci.comfy.org/?branch=4303%2Fmerge

@mcmonkey4eva mcmonkey4eva removed the Run-CI-Test This is an administrative label to tell the CI to run full automatic testing on this PR now. label Sep 16, 2024
@mcmonkey4eva
Copy link
Contributor

Comfy looked this over and did some testing, and found a bug that didn't surface in my own testing and attempts to cleanup (openaimodel line 649 edit is wrong), and after some discussion we reached the conclusion that this type of broad large alteration should not come from an external PR, or if you do send a PR it should be one file at a time, and very thoroughly validated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good PR This PR looks good to go, it needs comfy's final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants