-
-
Notifications
You must be signed in to change notification settings - Fork 259
Factory pattern for model components #1211
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
Conversation
|
@O-J1 requested review from you because of possible implications of how modules are imported in this PR here: OneTrainer/modules/util/create.py Line 53 in c85e20e
ruff would remove regular imports because the modules aren't used anywhere - except that they register themselves when they are imported. any downsides to importing just all python files that are in these directories? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might:
- Get some ruff false positives but I dont believe anything will be auto removed
- Import errors will still be fatal (same as current)
- Some impact to startup delays (same as current) because it all has to import and execute, even if it wont be used.
P.S I still think we should try to do conditional imports for the model actually neeed(?) havent ascertained if this would even be possible yet though
I don't know if we should do that, but I can say that this PR doesn't change that. |
dxqb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has been tested as part of the Flux2 PR
This should make it easier to implement new models by having the model-specific code less spread around the entire codebase
includes #1210