-
Notifications
You must be signed in to change notification settings - Fork 182
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
Migrate tests to pytest + add GA #127
Conversation
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.
Looks good to me as initial PR.
@tgale96 any other tests you want to include / would be helpful?
Updates:
To do: |
Let's do in follow-on PR
Yes I think so. |
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.
Eitan, you shouldn't resolve conversations unless you address the comment or answer the question
Have you tested setup.py? Are all tests supposed to be GPU tests?
Sorry, I shouldn't have resolved those conversations. In particular, I've now addressed your comment about using v0.1.0 of ci-testing. I fixed some bugs and we are now using v0.1.0 for ci-testing and have now resolved that comment. Wanted to make sure that is alright. I do not have explicit tests for the setup.py. But when we install MegaBlocks to run all of our tests, we implicitly need the setup.py to work correctly. Are there any specific tests you recommend adding for the setup.py? And yes, all tests are currently GPU tests. We are mainly testing two things: ops and layers. Ops are wrappers around low level kernels; these kernels only work on GPUs. Layers are different MoE configurations where we need to perform inference; again, this is done on GPUs because it is inference. There is not much more in MegaBlocks beyond this and so there are not many non-GPU tests to run. Happy to add non-GPU tests if you have any in mind. Also, this is just meant to be a first PR. I'll add more tests for more complete coverage in future PRs. |
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.
Have you manually run setup and GPU tests? |
Yes. |
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.
Thanks for the PR!
In this PR, I
pyproject.toml
to handle us importingtorch
insetup.py
pyproject.toml
add settings/configurations forpytest
andcoverage
.setup.py
dev requirements for testingsetup.py
, use up to date versions ofgrouped_gemm
andstk
packages that properly handle build isolation (@tgale96 merged my PRs into those libraries.).gitignore
, based onComposer
's.gitignore
Makefile
to be used with testing, based onComposer
'sMakefile
ci-testing
repo, based onComposer
's GAAll the tests currently pass. And you can see the Github Action working properly in my fork here.
Notes: