Skip to content

Code deduplication#1143

Merged
dxqb merged 1 commit intoNerogar:mergefrom
dxqb:generic
Nov 27, 2025
Merged

Code deduplication#1143
dxqb merged 1 commit intoNerogar:mergefrom
dxqb:generic

Conversation

@dxqb
Copy link
Collaborator

@dxqb dxqb commented Nov 17, 2025

This is an attempt to de-duplicate identical code between models,

  • but keep the OneTrainer object model unchanged
  • still have the possibility to write model-specific classes if that is necessary for future models
  • this makes changes easier for future PRs. as an example, one of the existing PRs requires passing quantization settings through the model loader

This PR is only model loaders, but the principle could be applied to other parts of the object model.
This PR uses class constructors, similar to C++ templates. An alternative to this way would be to use base classes and mixins, but I feel this way is cleaner and less likely to break things. Feedback welcome

@dxqb dxqb requested a review from Nerogar November 17, 2025 15:23
dxqb added a commit to dxqb/OneTrainer that referenced this pull request Nov 24, 2025
@Nerogar
Copy link
Owner

Nerogar commented Nov 27, 2025

I haven't tested the change, but it's much shorter and still readable. I usually prefer inheritance and mixins. But in this case I think it's ok to deviate from the usual style if it makes the code easier to maintain.

@dxqb dxqb marked this pull request as ready for review November 27, 2025 19:08
@dxqb dxqb added the merging last steps before merge label Nov 27, 2025
@dxqb dxqb changed the base branch from master to merge November 27, 2025 20:17
@dxqb
Copy link
Collaborator Author

dxqb commented Nov 27, 2025

Thanks! It has been merged into #1034 which people have tested, so it should already be tested reasonably well.

@dxqb dxqb merged commit a1afcd3 into Nerogar:merge Nov 27, 2025
1 check passed
dxqb added a commit that referenced this pull request Nov 28, 2025
@dxqb dxqb deleted the generic branch November 28, 2025 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merging last steps before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants