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.

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

Melt windows fix #137

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Melt windows fix #137

wants to merge 5 commits into from

Conversation

TomMelt
Copy link
Member

@TomMelt TomMelt commented Jun 19, 2024 •
edited
Loading

fixes #124

Windows was not previously exporting the library symbols and creating the dll correctly.

This is now resolved by the following addition to CMakeLists.txt

FTorch/src/CMakeLists.txt

Lines 6 to 9 in 7856a33

if (WIN32)
set (CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
set (BUILD_SHARED_LIBS TRUE)
endif ()

Furthemore, there was an issue that the library was built with a different integer size than that used when compiling example programs, despite no difference in compiler option. We have now forced the library to use int32 explicitly. This can be modified by the user, modifying the following line

FTorch/src/ftorch.fypp

Line 34 in 7856a33

integer, parameter :: ftorch_int = int32 ! set integer size for FTorch library

Tasks:

  • fix cmake windows build issue
  • fix int64/int32 issue properly (we have decided to build int32 by default. Can be overridden by user.
  • update documentation to reflect new changes

@TomMelt TomMelt added the bug Something isn't working label Jun 19, 2024
@TomMelt TomMelt self-assigned this Jun 19, 2024
@TomMelt TomMelt mentioned this pull request Jun 24, 2024
Open
@jatkinson1000
Copy link
Member

jatkinson1000 commented Sep 9, 2024 •
edited by TomMelt
Loading

Discussion:

@TomMelt TomMelt mentioned this pull request Nov 18, 2024
Open
4 tasks
@TomMelt
feat: set integer size for FTorch ftorch_int
7856a33 
Integer size for FTorch can now be controlled using parameter
`ftorch_int`.

Currently, the default is set to `int32` but this could be changed if
required.

This fixes an issue on windows with the latest version of ifx/ifort
which builds with a different integer type leading to the following
error:

```
error #6284: There is no matching specific function for this generic function reference.   [TORCH_TENSOR_FROM_ARRAY]
   in_tensors(1) = torch_tensor_from_array(in_data, tensor_layout, torch_kCPU)
-------------------^
```
jwallwork23
jwallwork23 requested changes Nov 19, 2024
View reviewed changes
Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

This all looks reasonable and in agreement with what we discussed in the meeting, thanks!

The remaining thing to do is to document this, as mentioned in the checklist for the PR.

jatkinson1000
jatkinson1000 requested changes Nov 19, 2024
View reviewed changes
Copy link
Member

@jatkinson1000 jatkinson1000 left a comment

Choose a reason for hiding this comment

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

Discussed in meeting, small comments on looking at the code, looks like just docs left.

src/CMakeLists.txt Outdated
Comment on lines 6 to 10
if (WIN32)
set (CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS TRUE)
set (BUILD_SHARED_LIBS TRUE)
endif ()

Copy link
Member

Choose a reason for hiding this comment

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

A small comment to explain presence of this clause?

Copy link
Member Author

Choose a reason for hiding this comment

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

done in b3b50a1

src/ftorch.fypp Outdated
@@ -514,7 +516,7 @@ contains
integer(c_int), parameter :: c_dtype = ${enum_from_prec(PREC)}$ !! Data type
integer(c_int64_t) :: strides(${RANK}$) !! Strides for accessing data
integer(c_int), parameter :: ndims = ${RANK}$ !! Number of dimension of input data
integer :: i
integer(ftorch_int) :: i
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
integer(ftorch_int) :: i
integer(ftorch_int) :: i

Nit

Copy link
Member Author

Choose a reason for hiding this comment

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

I did one better. See 594e938. Hope you like it 🤞

@TomMelt
Copy link
Member Author

TomMelt commented Nov 19, 2024

This all looks reasonable and in agreement with what we discussed in the meeting, thanks!

The remaining thing to do is to document this, as mentioned in the checklist for the PR.

thanks. Just done in d64358c

@jwallwork23 jwallwork23 mentioned this pull request Nov 19, 2024
Draft
6 tasks
@jatkinson1000 jatkinson1000 mentioned this pull request Nov 19, 2024
Open
jwallwork23
jwallwork23 approved these changes Nov 21, 2024
View reviewed changes
Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the docs, @TomMelt. Just one minor request then good to go!

pages/troubleshooting.md
integer, parameter :: ftorch_int = int64 ! set integer size for FTorch library
```

Finally, you will need to run `fypp` e.g.,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess users may also need to install this because it isn't in the set of minimal requirements.

pages/troubleshooting.md

Finally, you will need to run `fypp` e.g.,
```bash
fypp src/ftorch.fypp > src/ftorch.F90
Copy link
Contributor

Choose a reason for hiding this comment

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

Not important but FYI this also works without the >.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jwallwork23 TIL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jatkinson1000 jatkinson1000 jatkinson1000 requested changes

@jwallwork23 jwallwork23 jwallwork23 approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@TomMelt TomMelt

Labels
bug Something isn't working hackathon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows Installation Issue
3 participants
@TomMelt @jatkinson1000 @jwallwork23