Skip to content

Update check all set #174

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

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Conversation

LaurentMazare
Copy link
Contributor

When calling m.update(parameters: ..., verify: .all) on a module, there is no check that is performed to ensure that all the parameters under m have been set. This has bitten me a couple times when porting a model to mlx-swift, and I think it's related to this todo in Module.swift.

This PR adds some support to check also for this. A couple comments:

  • Most of the change is in the recursive call to .apply where the .value can be .none for the case where we don't have anything in the parameters but have to go through the modules to check that there is no parameter to be set.
  • When handling the .dictionary case , the iteration has been changed so that we ensure to go over all the model items rather than going through all the parameters.
  • This modifies the default behavior so it's a breaking change. Another possibility could be to set .all not to include the new .allModelKeysSet and have a new .strict flag that is .all + .allModelKeysSet.
  • This only affects the parameters update, not the update with modules.
  • A dedicated test has been added.

Any feedback on this is very welcome.

@awni awni requested a review from davidkoski December 9, 2024 16:53
Copy link
Collaborator

@davidkoski davidkoski left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the improvement!

I think having .all cover this case is probably a good idea

@davidkoski davidkoski merged commit 15f12e4 into ml-explore:main Dec 9, 2024
3 checks passed
@LaurentMazare
Copy link
Contributor Author

Thanks for the prompt review & merge!

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.

2 participants