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

Reapply "[FRONTEND] added support for tuples (#5220)" #3043

Merged
merged 12 commits into from
Dec 21, 2024

Conversation

anmyachev
Copy link
Contributor

@anmyachev anmyachev commented Dec 18, 2024

This reverts commit 492ea92.

Summary of changes:

  • support for tuples
  • constexprs are now also part of the signature. The format is: signature={..., 'o': "*fp32", '[Name of constexpr]': 'constexpr'}
  • For ASTSource there is parameter name change: constants -> constexprs
  • New nesting level has appeared for the main properties (defined in _add_common_properties): divisibility_16, equal_to_1 and equal_to_none.

Inductor CI:

anmyachev and others added 8 commits December 18, 2024 20:41
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
@anmyachev
Copy link
Contributor Author

On Triton's side I think I fixed everything.

For Torch Inductor, even considering that I made several changes for it, this is still not enough and the full set of inductor tests does not pass. Considering the number of changes, I believe it is good idea to split it into another PR.

@anmyachev anmyachev marked this pull request as ready for review December 19, 2024 20:57
@anmyachev anmyachev requested review from alexbaden and whitneywhtsang and removed request for alexbaden December 19, 2024 20:57
@alexbaden
Copy link
Contributor

For Torch Inductor, even considering that I made several changes for it, this is still not enough and the full set of inductor tests does not pass. Considering the number of changes, I believe it is good idea to split it into another PR.

I agree. I think we should remove the PyTorch patch from this PR and keep it in a separate branch since the inductor tests will fail either way.

Copy link
Contributor

@whitneywhtsang whitneywhtsang left a comment

Choose a reason for hiding this comment

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

According to the discussion, are we removing all debug prints and PyTorch patches in this PR?

Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
@anmyachev
Copy link
Contributor Author

@alexbaden I can't completely remove the patch, because even the main tests will stop working. We have at least one test in the main set that uses PyTorch inductor:

ZE_AFFINITY_MASK=0 python pytorch/benchmarks/dynamo/huggingface.py --accuracy --float32 -dxpu -n10 --no-skip --dashboard --inference --freezing --total-partitions 1 --partition-id 0 --only AlbertForMaskedLM --backend=inductor --timeout=4800 --output=$(pwd -P)/inductor_log.csv

@whitneywhtsang I removed debug things from the Triton code, but I intend to leave them in the patch, because they will still be useful for fixing the inductor tests (at least they highlight places that are affected by the change in the Triton interface).

UPD: CI is not working now because there is no more space on the runners: /runner/_work/intel-xpu-backend-for-triton/intel-xpu-backend-for-triton/pytorch/third_party/ideep/mkl-dnn/src/gpu/intel/jit/gemm/gen_gemm_kernel_generator.cpp:27738:1: fatal error: error writing to /tmp/ccXBqNWT.s: No space left on device. cc @kwasd @pbchekin

@anmyachev
Copy link
Contributor Author

@pbchekin @kwasd please ping me when it makes sense to try to restart CI

@pbchekin
Copy link
Contributor

@pbchekin @kwasd please ping me when it makes sense to try to restart CI

Please restart.

@anmyachev
Copy link
Contributor Author

@pbchekin @kwasd please ping me when it makes sense to try to restart CI

Please restart.

Unfortunately it doesn't work

@pbchekin
Copy link
Contributor

Unfortunately it doesn't work

Ops, sorry for that. Let me fix the problem, merge #3053, then I restart the workflow.

@anmyachev
Copy link
Contributor Author

Test with pip failed in the Inductor test suite, and this happened because pytorch is not rebuilt with the new patch in this workflow. So I'm going to merge this pull request, assuming that as soon as new PyTorch wheels become available for this flow, it should go through successfully.

@anmyachev anmyachev merged commit 2347dba into main Dec 21, 2024
5 of 6 checks passed
@anmyachev anmyachev deleted the amyachev/issue2979 branch December 21, 2024 10:46
anmyachev added a commit that referenced this pull request Dec 21, 2024
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
whitneywhtsang pushed a commit that referenced this pull request Dec 21, 2024
Signed-off-by: Anatoly Myachev <anatoly.myachev@intel.com>
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.

4 participants