8000 Remove dynamo suported check for Windows. by stellaraccident · Pull Request #111313 · pytorch/pytorch · GitHub
[go: up one dir, main page]
Skip to content

Remove dynamo suported check for Windows. #111313

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 14 commits into from
Closed

Remove dynamo suported check for Windows. #111313

wants to merge 14 commits into from

Conversation

stellaraccident
Copy link
Contributor
@stellaraccident stellaraccident commented Oct 14, 2023 •
edited
Loading

We've had developers using Dynamo via torch.compile and torch._dynamo.export for months on Windows via SHARK (by just deleting these lines locally). The Windows check is moved to just the inductor backend, since it is based on Triton not supporting Windows.

Progress on #90768

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng

@pytorch-bot
Copy link
pytorch-bot bot commented Oct 14, 2023 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/111313

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure

As of commit 0689a86 with merge base 8da2f81 (image):

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla
Copy link
linux-foundation-easycla bot commented Oct 14, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@Chillee
Copy link
Collaborator
Chillee commented Oct 14, 2023

Oh this is a good point. I think it mostly reflects that Triton does not work on Windows. We should move the check to the Inductor backend and not put it at the top level for Dynamo.

@stellaraccident
Copy link
Contributor Author
stellaraccident commented Oct 14, 2023 •
edited
Loading

Added the check to the inductor backend. I imagine that there are also some tests that can have their Windows excludes removed with this?

(took a quick look and it seems reasonable as-is)

@dan-garvey
Copy link

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label Oct 15, 2023
@janeyx99 janeyx99 requested review from jansel and avikchaudhuri October 16, 2023 22:10
@janeyx99 janeyx99 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 16, 2023
jansel
jansel requested changes Oct 17, 2023
View reviewed changes
Copy link
Contributor
@jansel jansel left a comment

Choose a reason for hiding this comment

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

Looks like lints are failing, you should be able to fix with:

make setup_lint
lintrunner -a

@stellaraccident
Copy link
Contributor Author

Looks like lints are failing, you should be able to fix with:

make setup_lint
lintrunner -a

Thanks. Applied the lint patch.

@jansel
Copy link
Contributor
jansel commented Oct 22, 2023

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased dynamo_works_on_windows onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout dynamo_works_on_windows && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the dynamo_works_on_windows branch from b4305b4 to 5129d40 Compare October 22, 2023 23:26
@jansel
Copy link
Contributor
jansel commented Oct 22, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 22, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 1 jobs have failed, first few of them are: trunk / win-vs2019-cpu-py3 / test (default, 2, 3, windows.4xlarge.nonephemeral)

Details for Dev Infra team Raised by workflow job

jansel
jansel requested changes Oct 23, 2023
View reviewed changes
Copy link
Contributor
@jansel jansel left a comment

Choose a reason for hiding this comment

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

@stellaraccident the test failure above looks real

@stellaraccident
Copy link
Contributor Author

@stellaraccident the test failure above looks real

Thanks - can add it to my list to triage. Does anyone know how to trigger these specific tests on presubmit?

@angelayi angelayi mentioned this pull request Nov 21, 2023
Closed
@nirvedhmeshram
Copy link

From the error log:

torch._dynamo.exc.Unsupported: 'skip function cond in file C:\actions-runner\_work\pytorch\pytorch\build\win_tmp\build\torch\_higher_order_ops\cond.py'', skipped according skipfiles.SKIP_DIRS'

It looks like the forced inlined file torch._higher_order_ops.cond doesn't get inlined correctly. Can anyone run this unit tests with TORCH_LOGS="+dynamo" and share output logs here?

Here is a gist of the output logs https://gist.github.com/nirvedhmeshram/69e1f8499e3c2a8341563df7e4ae6182

@yanboliang
Copy link
Contributor

Can you run the the following code and share the output?

>>> import torch
>>> torch.cond.__code__
<code object cond at 0x7f44d43c5a50, file "/home/ybliang/local/pytorch/torch/_higher_order_ops/cond.py", line 36>
>>> from functorch.experimental.control_flow import cond
>>> cond.__code__
<code object cond at 0x7f44d43c5a50, file "/home/ybliang/local/pytorch/torch/_higher_order_ops/cond.py", line 36>

@nirvedhmeshram
Copy link

Can you run the the following code and share the output?

>>> import torch
>>> torch.cond.__code__
<code object cond at 0x7f44d43c5a50, file "/home/ybliang/local/pytorch/torch/_higher_order_ops/cond.py", line 36>
>>> from functorch.experimental.control_flow import cond
>>> cond.__code__
<code object cond at 0x7f44d43c5a50, file "/home/ybliang/local/pytorch/torch/_higher_order_ops/cond.py", line 36>

looks ok to me

>>> import torch
>>> torch.cond.__code__
<code object cond at 0x000001F241E78DB0, file "D:\shark_experiments\pytorch\torc
h\_higher_order_ops\cond.py", line 36>
>>> from functorch.experimental.control_flow import cond
>>> cond.__code__
<code object cond at 0x000001F241E78DB0, file "D:\shark_experiments\pytorch\torc
h\_higher_order_ops\cond.py", line 36>

@yanboliang
Copy link
Contributor

@nirvedhmeshram This is weird, as we already put it in the force inline list:
https://github.com/pytorch/pytorch/blob/23b030a79caf88007eb7a17eaa14e2368eed8b06/torch/_dynamo/skipfiles.py#L173C12-L173C29

And it works well on other platform, not sure why it was wrapped as SkipFilesVariable on win, which is supposed to UserFunctionVariable.

@nirvedhmeshram
Copy link

@nirvedhmeshram This is weird, as we already put it in the force inline list: https://github.com/pytorch/pytorch/blob/23b030a79caf88007eb7a17eaa14e2368eed8b06/torch/_dynamo/skipfiles.py#L173C12-L173C29

And it works well on other platform, not sure why it was wrapped as SkipFilesVariable on win, which is supposed to UserFunctionVariable.

Its because of many places using "/" as seperator in file paths while windows need a different seperator so indeed the kind of fixes done here https://github.com/pytorch/pytorch/pull/109677/files are passing the tests for me. I need to apply the fixes properly and then will push up the fix, hopefully in a few hours.

@yanboliang
Copy link
Contributor

Its because of many places using "/" as seperator in file paths while windows need a different seperator so indeed the kind of fixes done here https://github.com/pytorch/pytorch/pull/109677/files are passing the tests for me. I need to apply the fixes properly and then will push up the fix, hopefully in a few hours.

That makes sense, but I'm curious why our CI doesn't capture this, since the skip/inline rules are used everywhere.

@nirvedhmeshram
Copy link
nirvedhmeshram commented Feb 8, 2024 •
edited
Loading

I think this is dynamo specific (at least the changes I am making are all in /torch/_dynamo) and dynamo was disabled on windows so it never hit these issues.

@nirvedhmeshram nirvedhmeshram force-pushed the dynamo_works_on_windows branch from 1da4a3f to e2513fd Compare February 8, 2024 02:08
stellaraccident and others added 13 commits February 8, 2024 11:20
@stellaraccident @nirvedhmeshram
Remove dynamo suported check for Windows.
1259156 
We've had developers using Dynamo via torch.compile and torch._dynamo.export for months on Windows via SHARK (by just deleting these lines locally). I expect this reflects that some backends do not support Windows yet? If so, could the check be moved to those backends vs blocking all use?
@nirvedhmeshram
rebase
a957bb9
@stellaraccident @nirvedhmeshram
Apply lint patch
684846c
@nirvedhmeshram
make sure tests that are disabled for windows, stay disabled
c335c9a
@nirvedhmeshram
check if correct commpile_support check solves the failing tests
3ef58ab
@nirvedhmeshram
re-enabling the initially disabled tests, also adding windows compile…
9df7198 
… support for cpu
@nirvedhmeshram
cond is not supported on windows
ed1ae3c
@nirvedhmeshram
fix wrong skip logic
ad35ab8
@nirvedhmeshram
lint
9484469
@nirvedhmeshram
fix file seperator paths for windows
9c5d56c
@nirvedhmeshram
add back cond
7de8897
@nirvedhmeshram
currently no compile support for windows as this uses inductor backen…
dbffaba 
…d which we have disabled
@nirvedhmeshram
apply os.sep to trace_rules.py
ee4dd33
@nirvedhmeshram nirvedhmeshram force-pushed the dynamo_works_on_windows branch from ec13655 to ee4dd33 Compare February 8, 2024 17:26
@nirvedhmeshram
Copy link
nirvedhmeshram commented Feb 8, 2024 •
edited
Loading

I had to disable test_export_then_compile_tensor_ctor test for windows as it is using inductor which is not set up to find the cpp compiler as of now. Since this patch is trying to enable dynamo without trying to get inductor working, I hope that is okay. (Will ask for review after the CI passes.)

pytorchmergebot pushed a commit that referenced this pull request Feb 8, 2024
@gs-olive @stellaraccident @pytorchmergebot
Windows Dynamo Error Removal CI Check (#115969)
45e7af5 
Rebase of #111313 onto `main`, for CI validation

Co-authored-by: Stella Laurenzo <stellaraccident@gmail.com>
Pull Request resolved: #115969
Approved by: https://github.com/ezyang
@monorimet
Copy link

Looking at the CI failure here,

Public FX API(s) ['torch.fx.passes.backends.cudagraphs.CudaGraphsSupport', 'torch.fx.passes.backends.cudagraphs.partition_cudagraphs', 'torch.fx.passes.infra.partitioner.CapabilityBasedPartitioner', 'torch.fx.passes.infra.partitioner.Partition'] introduced but not given a backwards-compatibility classification! Please decorate these API(s) with `@torch.fx._compatibility.compatibility` to specify BC guarantees.

can these decorators simply be added as suggested, or is there more to this failure that isn't being addressed in the logs?

@stellaraccident
Copy link
Contributor Author

Closing because we think that #115969 addresses this.

clee2000 pushed a commit that referenced this pull request Feb 14, 2024
@gs-olive @stellaraccident @clee2000
Windows Dynamo Error Removal CI Check (#115969)
4df1b50 
Rebase of #111313 onto `main`, for CI validation

Co-authored-by: Stella Laurenzo <stellaraccident@gmail.com>
Pull Request resolved: #115969
Approved by: https://github.com/ezyang
pytorchmergebot pushed a commit that referenced this pull request Feb 14, 2024
@gs-olive @stellaraccident @pytorchmergebot
Windows Dynamo Error Removal CI Check (#115969)
e0f6fa6 
Rebase of #111313 onto `main`, for CI validation

Co-authored-by: Stella Laurenzo <stellaraccident@gmail.com>
Pull Request resolved: #115969
Approved by: https://github.com/PaliC, https://github.com/thiagocrepaldi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jansel jansel jansel requested changes

@avikchaudhuri avikchaudhuri Awaiting requested review from avikchaudhuri

@Chillee Chillee Awaiting requested review from Chillee

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request module: dynamo open source release notes: dynamo Stale triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@stellaraccident @Chillee @dan-garvey @jansel @pytorchmergebot @nirvedhmeshram @yanboliang @monorimet @pytorchbot @janeyx99
0