-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[SLM] Add unit tests for SLM to Relax exporter #16784
[SLM] Add unit tests for SLM to Relax exporter #16784
Conversation
Follow-up to apache#16777, add unit tests demonstrating desired behavior.
Prior to this commit, the weights used by `nn.Module` instances were required to be `nn.Parameter` instances. This commit allows the weights to instead be `nn.Tensor` instances, defined in terms of other `nn.Parameter` weights. This allows a model to define both the original weights that would be present in an external checkpoint (e.g. a Pytorch or Safetensors file), and the pre-processing that should be performed on those weights. This is a re-implementation of apache#16757, which was reverted in apache#16777. The re-implementation preserves the handling of dynamic shapes specified as python strings, enabling the test cases that were added in apache#16784.
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.
It's good to have test cases, especially when they explain the intended functionality very well. A tutorial featuring based on these examples (perhaps literally generated from the test cases) might be a good investment of time as well, especially if it can be made more visible.
|
||
|
||
def test_custom_module(): | ||
"""A module may be exported from nn.Module to Relax""" |
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.
Perhaps the comment should differ in soem way from that in the above test.
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.
Good call, and I've updated the docstring to remove the copy/paste duplicate.
Thank you, and that was in part my goal with the sequence of unit tests. Whenever possible, I prefer test cases that double as mini-tutorials, since then they are less likely to become stale than full tutorials. (Though I agree that it would be beneficial to have a tutorial that follows user-focused flow, rather than the feature-focused flow of these unit tests.) |
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.
Thank you @Lunderberg 🙏
* [SLM] Add unit tests for SLM to Relax exporter Follow-up to apache#16777, add unit tests demonstrating desired behavior. * Updated docstrings based on review comment
Follow-up to #16777, add unit tests demonstrating desired behavior.