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

[SLM] Add unit tests for SLM to Relax exporter #16784

Merged
merged 2 commits into from
Mar 29, 2024

Conversation

Lunderberg
Copy link
Contributor

Follow-up to #16777, add unit tests demonstrating desired behavior.

Follow-up to apache#16777, add unit tests
demonstrating desired behavior.
@Lunderberg
Copy link
Contributor Author

@tqchen @MasterJH5574

Lunderberg added a commit to Lunderberg/tvm that referenced this pull request Mar 25, 2024
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.
Copy link
Contributor

@slyubomirsky slyubomirsky left a 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"""
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@Lunderberg
Copy link
Contributor Author

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.

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.)

Copy link
Contributor

@MasterJH5574 MasterJH5574 left a comment

Choose a reason for hiding this comment

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

Thank you @Lunderberg 🙏

@Lunderberg Lunderberg merged commit f8b9a5f into apache:main Mar 29, 2024
20 checks passed
@Lunderberg Lunderberg deleted the slm_add_unit_tests_for_nn_exporter branch March 29, 2024 12:52
thaisacs pushed a commit to thaisacs/tvm that referenced this pull request Apr 3, 2024
* [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
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