NumPy support in torch.compile #106211
NumPy support in torch.compile #106211
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/106211
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 4 Unrelated FailuresAs of commit 237489d: NEW FAILURE - The following job has failed:
BROKEN TRUNK - The following job failed but were present on the merge base 22bc08d:👉 Rebase onto the `viable/strict` branch to avoid these failures
UNSTABLE - The following jobs failed but were likely due to flakiness present on trunk and has been marked as unstable:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
|
For the reviewers, could you have a look at commits you feel comfortable reviewing? All the large ones are just either automated changes or trying to make lintrunner pass. |
|
I think all authors need to sign CLA |
|
Does this mean numpy becomes a dependency of pytorch? |
This should be fine, we've included code from many other compatibly-licensed projects in the code base before. I suggest adding an entry in
No, no dependency is added in this PR, the changes are self-contained. |
|
I notice there are lots of places where tests rely on pytest and pytest tools. If these tests are to be upstreamed, their test infra dependencies will have to be reconciled with our current test infra--we have our own nuanced equivalents of parametrize() (see ) as well as xfails and our general OpInfos (see ). In general, we don't want to introduce a pytest dependency + we have special handling in our test infra so bringing consistency there will be important.For example, I have not gotten a chance to go through the numpy op testing, but we probably want to:
|
|
@janeyx99 these tests are copied with minor edits from NumPy's test suite. I don't think it'd be realistic to port them to our own suite. In fact, we tried very hard to make them pass with as few modifications as possible, as to be sure that we had implemented a compat layer that was replicating NumPy faithfully. That being said, as I mentioned in in the OP, I am still to remove imports and dependencies on internal NumPy objects, as to not create dependencies on non-public NumPy objects. If people really feel strongly about it, I could try to make the tests not depend on NumPy at all, but that would require significantly more effort, of course. As to the |
|
Also, we discussed doing something like having our own OpInfos at the beginning of this project and we discarded it. Getting to the point where we are now when it comes to OpInfos would involve much more budget than we had for this project (the current OpInfos were implemented by many engineers throughout many months of work) so, even if it were cleaner, I don't think that the cost/benefit would be on our side. |
0c7925f to
9cb447c
Compare
|
Supporting @lezcano here; I also agree that it would be a lot of "makework" to "port" Numpy's test suite to PyTorch style. In fact, I have the opposite question, which is that in an ideal world, we would occasionally take updates from Numpy's test suite as Numpy evolves and we evolve with it. Is the test suite identical enough that this would be possible to do? (Removing the dependencies to Numpy internals would make this harder! But maybe it is more important to be broadly compatible with many versions of Numpy than it is to be able to copy paste in the Numpy suite?) |
|
We discussed the point of automatising the process of bringing tests from the NumPy test suite, but it's not particularly easy. To put together this testing suite, we went through a reasonable curation of these tests, where some were skipped as they used features we were never going to support (endianness, arrays of strings...), those that we would potentially like to eventually support (we would xfail these) and we edited some removing the parts we were not going to support but leaving the rest (e.g. tests that would iterate over all the datatypes, we would remove long double and int16 and so on, while keeping the rest). I think it'd be rather difficult to separate the signal from the noise if this process were to be automatised. |
|
Disappointing but understandable |
|
cc @williamwen42 for all the dynamo errors in 3.11. I will try to file self-contained issues next week. |
|
@lezcano Yea I don’t feel strongly at all about having the test suite be strictly following PyTorch test infra (vs sticking to numpy’s) and what you and Ed said makes sense regarding expanding the scope of our tests. My observation is moreso that this change would certainly introduce lots of tests that vary from what PyTorch has been used to. If our current test infra (and people on the DevInfra team would know more) already automatically supports these tests with our features (like disable bot, test reporting, …), then that’s great! If not, then certain things may have to change in PyTorch test infra or there might be constraints worth bringing to light earlier rather than later. |
…el ends in _impl'
|
oopsie daisy |
|
@ezyang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@ezyang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1 similar comment
|
@ezyang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
This reverts commit a9dca53.
|
This is failing windows smoke tests: https://github.com/pytorch/pytorch/actions/runs/5830091528/job/15823773511#step:13:471 |
As discussed in the review of #106211, remove several noops (#106211 (review) and #106211 (review)). Pull Request resolved: #107596 Approved by: https://github.com/lezcano
RFC: pytorch/rfcs#54
First commit is the contents of https://github.com/Quansight-Labs/numpy_pytorch_interop/
We have already been using this in core for the last few months as a external dependency. This PR pulls all these into core.
In the next commits, I do a number of things in this order
torch_npand simply rely on the upstreamed codeMissing from this PR (but not blocking):
All the tests in
tests/torch_nptake about 75s to run.This was a work by @ev-br, @rgommers @honno and I. I did not create this PR via ghstack (which would have been convenient) as this is a collaboration, and ghstack doesn't allow for shared contributions.
cc @mruberry @rgommers @albanD @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @ipiszy @ngimel @yf225 @chenyang78 @kadeng @muchulee8 @aakhundov @anijain2305 @ev-br