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

Windows E2E #689

Merged
merged 28 commits into from
Aug 29, 2024
Merged

Windows E2E #689

merged 28 commits into from
Aug 29, 2024

Conversation

makslevental
Copy link
Collaborator

@makslevental makslevental commented Aug 19, 2024

This PR adds Windows E2E support.

@makslevental makslevental force-pushed the makslevental/windows-e2e branch 2 times, most recently from 4f615fb to eb62bc8 Compare August 20, 2024 04:25
@makslevental makslevental force-pushed the makslevental/windows-e2e branch 8 times, most recently from 18065f0 to a25f441 Compare August 20, 2024 09:42
Copy link
Contributor

@newling newling left a comment

Choose a reason for hiding this comment

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

Just noticed it's still WIP so won't review yet

build_tools/ci/cpu_comparison/run_test.py Outdated Show resolved Hide resolved
@makslevental makslevental force-pushed the makslevental/windows-e2e branch 13 times, most recently from 3b435af to 28fdae4 Compare August 22, 2024 05:52
@makslevental makslevental force-pushed the makslevental/windows-e2e branch from b62e325 to a74fcde Compare August 29, 2024 03:11
@makslevental makslevental force-pushed the makslevental/windows-e2e branch 2 times, most recently from c9c7175 to 1fdd989 Compare August 29, 2024 04:08
@makslevental makslevental force-pushed the makslevental/windows-e2e branch from 1fdd989 to cac317d Compare August 29, 2024 04:16
@makslevental makslevental merged commit 0bc513c into main Aug 29, 2024
6 checks passed
@makslevental makslevental deleted the makslevental/windows-e2e branch August 29, 2024 04:49
- name: E2E correctness matmul test
run: |
source .venv/Scripts/activate
export XILINX_XRT=/c/Xilinx/XRT

Choose a reason for hiding this comment

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

On windows OS, all applications are supposed to uses xrt_coreutil.dll from c:\windows\system32. But this line make the CI computer use the xrt_coreutil.dll from /c/Xilinx/XRT.
This may cause all our tests get a different test result from a system deployed to general public

Copy link

Choose a reason for hiding this comment

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

Agree with Dezhi's statement. On Windows XILINX_XRT should not be set when running tests.

Copy link
Collaborator Author

@makslevental makslevental Nov 12, 2024

Choose a reason for hiding this comment

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

On Windows XILINX_XRT should not be set when running tests.

As far as I am aware, XRT is open source and thus we are free to use it according to how it best suits our needs. So we have our usecase and implementation and you have yours. If you feel so strongly, you are free to refactor your code so that it doesn't expose these env variables (and I am free to fork).

Anyway XRT is deprecated in iree-amd-aie so this entire discussion is moot.

Choose a reason for hiding this comment

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

@makslevental ,
What version of NPU KMD driver is installed on the windows computer used for CI?

Choose a reason for hiding this comment

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

On Windows XILINX_XRT should not be set when running tests.

As far as I am aware, XRT is open source and thus we are free to use it according to how it best suits our needs. So we have our usecase and implementation and you have yours. If you feel so strongly, you are free to refactor your code so that it doesn't expose these env variables (and I am free to fork).

Anyway XRT is deprecated in iree-amd-aie so this entire discussion is moot.

Based on what I see, removing XILINX_XRT works perfectly fine on my local system. There is a possible wrong setting on the CI computer

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please stop commenting on this PR. It is now 3 months since it was closed. If you have some current issue then create a new issue about it.

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.

6 participants