-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add Windows support to automatic installation script #371
Conversation
Thank you @otabuzzman for this patch. We will take a look in the following days. Regarding the errors in the unit-tests. some of them are expected and it depends on the driver user. But we will take a closer look as well. |
I issued the commit in question from my Macbook. There is another Git
configuration that incorrectly uses my real name instead of otabuzzman. I
will fix the problem when I have access to my MacBook on April 13th.
However, fixing the configuration will not change the commit in this PR.
Should I create a new one and you reject this PR?
Am Sa., 6. Apr. 2024 um 07:34 Uhr schrieb Juan Fumero <
***@***.***>:
… Thank you @otabuzzman <https://github.com/otabuzzman> for this patch. We
will take a look in the following days.
In the meantime, it seems some of the commits were done using another
account in which the CLA assistant detects that it has not been signed.
Regarding the errors in the unit-tests. some of them are expected and it
depends on the driver user. But we will take a closer look as well.
—
Reply to this email directly, view it on GitHub
<#371 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD7PMXEOSQ5HTABYBNNE7GDY36COFAVCNFSM6AAAAABFZVBGE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBQHE3TKOJUG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi Jürgen. I understand. Unfortunately, we can't merge if the CLA is not signed for all commits. So my take here, as you mentioned, is to open a new PR with the diff using the account/email that you have already used for the CLA for the previous merges. Sorry for the inconvenience. From our side, half of the team will be in a conference from mid this week, so we could start review next week too. |
No problem. I‘ll fix the commit and open a new PR. Have fun on your
conference. Regards, Jürgen.
Juan Fumero ***@***.***> schrieb am Mo. 8. Apr. 2024 um 08:57:
… Hi Jürgen. I understand. Unfortunately, we can't merge if the CLA is not
signed for all commits. So my take here, as you mentioned, is to open a new
PR with the diff using the account/email that you have already used for the
CLA for the previous merges. Sorry for the inconvenience.
From our side, half of the team will be in a conference from mid this
week, so we could start review next week too.
—
Reply to this email directly, view it on GitHub
<#371 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD7PMXBIYB4JWHRWMKJM5VLY4I5UDAVCNFSM6AAAAABFZVBGE6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBRHE4TINJTHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I have tested on Windows 11 with the OpenCL backend and it works perfectly. Thank you, this is great work. The tests that fail, many of them are expected, especially those related to SPIR-V and PTX. Thank you for the report too. On Linux, many of these are passing. |
I got an error when running the native test: ==================================================
Unit tests report
==================================================
{'[PASS]': 610, '[FAILED]': 10, '[UNSUPPORTED]': 45}
Coverage [PASS/(PASS+FAIL)]: 98.39%
Coverage [PASS/(PASS+FAIL+UNSUPPORTED)]: 91.73%
==================================================
Total Time(s): 179.11767625808716
NMAKE : fatal error U1077: 'python %TORNADO_SDK%\bin\tornado-test --ea --verbose' : return code '0x1'
Stop.Stop. |
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. I have minor comments.
if not os.path.exists(directoryName): | ||
## clone the repo with the OpenCL Headers | ||
import shutil |
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.
Why there is no need to create a temporal directory?
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.
There is no naming conflict between the top directory of the OpenCL header repository and other pre-existing files. Therefore, I don't think a temporary directory is necessary.
Furthermore, I skipped the removal of the OpenCL header directory after installation, just in case someone wants to look inside later for whatever reason... But after all, it's a matter of taste. If you prefer a temporal directory with removal, I'm fine with that.
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.
Ok, thanks for the clarification. Let's keep it then.
bin/tornadovm-installer
Outdated
__DIRECTORY_DEPENDENCIES__ = "etc/dependencies" | ||
__VERSION__ = "v1.0.1-dev" | ||
__DIRECTORY_DEPENDENCIES__ = os.path.join("etc", "dependencies") | ||
__VERSION__ = "v1.0.3" |
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.
[update] Good catch. Actually, it is the v.1.0.4-dev
. Let's change it to that.
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.
I set it to v1.0.4-dev
. Your value v0.1.0.4-dev
ist a typo, right? (note the 0 after the v).
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.
Yes, sorry. it was a typo.
Linux and OSx also work. |
I think this is expected behavior: The script The same goes for ==================================================
Unit tests report
==================================================
{'[PASS]': 566, '[FAILED]': 9, '[UNSUPPORTED]': 56}
Coverage [PASS/(PASS+FAIL)]: 98.43%
Coverage [PASS/(PASS+FAIL+UNSUPPORTED)]: 89.7%
==================================================
Total Time(s): 254.25732254981995
make: *** [tests] Error 1
[ec2-user@ip-172-31-18-17 TornadoVM]$ |
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.
is the bin/tornadovm-installer.cmd
file still needed? To my understanding, this script can be removed.
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.
LGMT. I tested it in Windows, mac and Linux. The only comment is if we deprecate the .\bin\tornadovm-installer.cmd
, to also update the installation.rst
file.
Question: Have you tried also the MSYS2
workflow?
Co-authored-by: Thanos Stratikopoulos <34061419+stratika@users.noreply.github.com>
I think it would be ok to deprecate
Good and obvious question. I didn't think of it and thus didn't test it with MSys2. Should I? The documentation does not foresee running bin\tornadovm-installer in the |
Yes, I feel the same. Let's remove it to keep it simple. Can you please include this change in the PR?
That's fine. I spoke with @jjfumero and we believe that Thank you very much @otabuzzman, once the script is removed we can proceed to merge it. |
Sure. Done.
I think that the section on manual Windows installation is worth keeping in an adapted version, and provided a proposal (my last commits). I removed the section on Windows issues as well since they were all related to |
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.
Thank you! LGTM
Improvements ~~~~~~~~~~~~~~~~~~ - [beehive-lab#369](beehive-lab#369): Introduction of Tensor types in TornadoVM API and interoperability with ONNX Runtime. - [beehive-lab#370](beehive-lab#370): Array concatenation operation for TornadoVM native arrays. - [beehive-lab#371](beehive-lab#371): TornadoVM installer script ported for Windows 10/11. - [beehive-lab#372](beehive-lab#372): Add support for ``HalfFloat`` (``Float16``) in vector types. - [beehive-lab#374](beehive-lab#374): Support for TornadoVM array concatenations from the constructor-level. - [beehive-lab#375](beehive-lab#375): Support for TornadoVM native arrays using slices from the Panama API. - [beehive-lab#376](beehive-lab#376): Support for lazy copy-outs in the batch processing mode. - [beehive-lab#377](beehive-lab#377): Expand the TornadoVM profiler with power metrics for NVIDIA GPUs (OpenCL and PTX backends). - [beehive-lab#384](beehive-lab#384): Auto-closable Execution Plans for automatic memory management. Compatibility ~~~~~~~~~~~~~~~~~~ - [beehive-lab#386](beehive-lab#386): OpenJDK 17 support removed. - [beehive-lab#390](beehive-lab#390): SapMachine OpenJDK 21 supported. - [beehive-lab#395](beehive-lab#395): OpenJDK 22 and GraalVM 22.0.1 supported. - TornadoVM tested with Apple M3 chips. Bug Fixes ~~~~~~~~~~~~~~~~~~ - [beehive-lab#367](beehive-lab#367): Fix for Graal/Truffle languages in which some Java modules were not visible. - [beehive-lab#373](beehive-lab#373): Fix for data copies of the ``HalfFloat`` types for all backends. - [beehive-lab#378](beehive-lab#378): Fix free memory markers when running multi-thread execution plans. - [beehive-lab#379](beehive-lab#379): Refactoring package of vector api unit-tests. - [beehive-lab#380](beehive-lab#380): Fix event list sizes to accommodate profiling of large applications. - [beehive-lab#385](beehive-lab#385): Fix code check style. - [beehive-lab#387](beehive-lab#387): Fix TornadoVM internal events in OpenCL, SPIR-V and PTX for running multi-threaded execution plans. - [beehive-lab#388](beehive-lab#388): Fix of expected and actual values of tests. - [beehive-lab#392](beehive-lab#392): Fix installer for using existing JDKs. - [beehive-lab#389](beehive-lab#389): Fix ``DataObjectState`` for multi-thread execution plans. - [beehive-lab#396](beehive-lab#396): Fix JNI code for the CUDA NVML library access with OpenCL.
Description
The PR adds Windows support to the
bin/tornadovm-installer
automatic installation script. The documentation (readthedocs) has been updated accordingly.Problem description
n/ a.
Backend/s tested
Mark the backends affected by this PR.
OS tested
Mark the OS where this PR is tested.
See results summary of test suite below.
Did you check on FPGAs?
If it is applicable, check your changes on FPGAs.
How to test the new patch?
On a Windows box:
setvars.cmd
python %TORNADO_SDK%\bin\tornado --devices
nmake /f Makefile.mak tests
Results summary (failures) of TornadoVM test suite.
Note: The suite was run twice consecutively on each configuration, with TornadoVM previously compiled for a single specific backend.
Windows 11 Home on Lenovo IdeaPad
OpenCL backend, 1st run
OpenCL backend, 2nd run
PTX backend, 1st run
PTX backend, 2nd run
SPIR-V backend, 1st run
SPIR-V backend, 2nd run
Amazon Linux on EC2 (g4dn.xlarge)
OpenCL backend, 1st run
OpenCL backend, 2nd run
PTX backend, 1st run
PTX backend, 2nd run
####Windows Server 2022 on EC2 (g4dn.xlarge)
Note: Missing SSL certificates required manual Maven download.
OpenCL backend, 1st run
OpenCL backend, 2nd run
PTX backend, 1st run
PTX backend, 2nd run
macOS Sonoma on Macbook Air (2015)
Note: Running the test suite required to remove enable assertions option in test target in Makefile. Probably due to Sonoma compatibility issues on unsupported MacBook Air (2015). Assert in question is in .../uk/ac/manchester/tornado/runtime/interpreter/TornadoVMInterpreter.java (device != null).
OpenCL backend, 1st run
OpenCL backend, 2nd run