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

Make clip always output dict in open_clip traininig #396

Merged
merged 12 commits into from
Jan 30, 2023

Conversation

gpucce
Copy link
Contributor

@gpucce gpucce commented Jan 30, 2023

This should make clip always use the output dict option when the training is done inside open_clip.

Partially addressing #390 and fixing #395

@rwightman

@gpucce gpucce marked this pull request as ready for review January 30, 2023 11:40
@rwightman
Copy link
Collaborator

rwightman commented Jan 30, 2023

@gpucce thanks, rather than is running in openclip, I think it should be output_dict: Optional[bool] = None ... if it's default of None, no change is made to the model default, otherwise, it will check attribute exists and pass through on creation

open_clip will set to True

@gpucce
Copy link
Contributor Author

gpucce commented Jan 30, 2023

@gpucce thanks, rather than is running in openclip, I think it should be output_dict: Optional[bool] = None ... if it's default of None, no change is made to the model default, otherwise, it will check attribute exists and pass through on creation

open_clip will set to True

changed to output_dict

@rwightman rwightman merged commit 009f06d into mlfoundations:main Jan 30, 2023
@rohan1561
Copy link

Hey, should the forward in the line 263 of training/train.py contain the output_dict argument? It throws an error for now..

@rwightman
Copy link
Collaborator

@rohan1561 there should be no output dict args in model fwd, don't know why that's still there, guess we missed one @gpucce

@gpucce
Copy link
Contributor Author

gpucce commented Feb 14, 2023

Hi, indeed there should not be, I will make a PR with also a test

@rwightman
Copy link
Collaborator

@gpucce I already pushed a quick fix, but PR for a test to cover this case would be good

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