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

Build example 3 if CUDA enabled #222

Merged
merged 15 commits into from
Jan 29, 2025
Merged

Build example 3 if CUDA enabled #222

merged 15 commits into from
Jan 29, 2025

Conversation

jwallwork23
Copy link
Contributor

@jwallwork23 jwallwork23 commented Jan 7, 2025 •
edited
Loading

Closes #208.
Closes #227.

This PR does several things:

  • Build example 3 if both CUDA and MPI are enabled.
  • Run example 3 in run_test_suite.sh if it has been built.
  • Address the example 3 Python issue raised in Device error in example 3 - GPU #227.
  • Fix missing imports in pt2ts.py script in example 3.
  • Rename the scripts in the MultiGPU example to start with multigpu, for consistency with the other examples.
  • Fix the numbering of the ctest commands in the examples CMakeLists (they were all ones).

I've attached the build script that I used to get this working locally:
build.sh.txt

@jwallwork23 jwallwork23 added the testing Related to FTorch testing label Jan 7, 2025
@jwallwork23 jwallwork23 self-assigned this Jan 7, 2025
@jwallwork23 jwallwork23 force-pushed the 208_multi-gpu-build branch 2 times, most recently from 6cc94c8 to 9d4aa86 Compare January 13, 2025 09:53
@jwallwork23 jwallwork23 marked this pull request as ready for review January 13, 2025 15:52
jatkinson1000
jatkinson1000 reviewed Jan 27, 2025
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.

@jwallwork23

Yes, agree with this.
However, a comment that I have seen before elsewhere - it would be good to avoid "double negatives" - the guard for running integration tests is based on the unit test flag. This requires some mental gymnastics (unless it's just me) and would be a problem if we wanted to add a third set of subtests for some reason in future.

I wonder if moving to having RUN_UNIT and RUN_INTEGRATION variables that default to true and are checked against for running the tests, and setting them to false where necessary in setup would be preferable?

This was referenced Jan 27, 2025
Closed
Closed
@jwallwork23
Copy link
Contributor Author

jwallwork23 commented Jan 27, 2025 •
edited
Loading

Yes, agree with this. However, a comment that I have seen before elsewhere - it would be good to avoid "double negatives" - the guard for running integration tests is based on the unit test flag. This requires some mental gymnastics (unless it's just me) and would be a problem if we wanted to add a third set of subtests for some reason in future.

I wonder if moving to having RUN_UNIT and RUN_INTEGRATION variables that default to true and are checked against for running the tests, and setting them to false where necessary in setup would be preferable?

Opened #259.

@jwallwork23 jwallwork23 mentioned this pull request Jan 27, 2025
Merged
@jwallwork23
Copy link
Contributor Author

Ah, just saw your commit 5935bb6. Apologies, looks like I updated the README on the wrong branch (see #258).

jwallwork23 and others added 15 commits January 29, 2025 10:20
@jwallwork23 @jatkinson1000
Build example 3 if CUDA and MPI enabled
a763b0e
@jwallwork23 @jatkinson1000
Put model on CUDA device in simplenet
eadc273
@jwallwork23 @jatkinson1000
Run example 3 if it's been built
e16c17d
@jwallwork23 @jatkinson1000
Add missing imports for pt2ts
1392a0a
@jwallwork23 @jatkinson1000
More helpful output for simplenet_infer_python
0129030
@jwallwork23 @jatkinson1000
Fix numbering in CMakeLists for examples
2392649
@jwallwork23 @jatkinson1000
Renaming in MultiGPU example; set up unit testing
6a532b5
@jwallwork23 @jatkinson1000
Raise error if no CUDA in example 3
e8b5780
@jwallwork23 @jatkinson1000
Lint
37d5da8
@jwallwork23 @jatkinson1000
Fix model filename passed to fortran
bec0734
@jwallwork23 @jatkinson1000
Do require mpi4py in Python script
ef0f938
@jwallwork23 @jatkinson1000
Drop ENABLE_MPI CMake argument
e0d4e45
@jatkinson1000
Bugfix: Update example 3 README with correct names of files simplenet…
8bcb9d0 
… -> multigpu.
@jatkinson1000
Bugfix: explicitly move loaded net to required device in multigpu python
a1f5688 
in example 3

If the net is saved from a particular cuda device it will be re-loaded
back to that cuda device. This causes issues if we are placing the
tensors on a different device, as is the case in the multigpu inference.
To fix this explicitly move the loaded net to the device.

We should look a little deeper about how this works on the Fortran/C++
side and verify where things are saved from/loaded to.
@jatkinson1000
Bugfix: Include missing CheckLanguage module in the CMakeLists of exa…
ef07fca 
…mple 3.
@jatkinson1000
Copy link
Member

@jwallwork23 rebased so this can go into maion with #258 being re-targeted.

Happy to approve once you have a quick glance over to check you are happy with the rebase.
I did check it in #262 as you saw.

@jwallwork23
Copy link
Contributor Author

@jwallwork23 rebased so this can go into maion with #258 being re-targeted.

Happy to approve once you have a quick glance over to check you are happy with the rebase. I did check it in #262 as you saw.

LGTM, thanks @jatkinson1000

jatkinson1000
jatkinson1000 approved these changes Jan 29, 2025
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.

Reviewed, tested, amended, and rebased.
Approving now @jwallwork23 and happy for you to merge in.

@jatkinson1000 jatkinson1000 mentioned this pull request Jan 29, 2025
Closed
@jwallwork23 jwallwork23 changed the title Build example 3 if CUDA and MPI enabled Build example 3 if CUDA enabled Jan 29, 2025
@jwallwork23 jwallwork23 merged commit c21040d into main Jan 29, 2025
6 checks passed
@jwallwork23 jwallwork23 deleted the 208_multi-gpu-build branch January 29, 2025 13:17
@jwallwork23 jwallwork23 mentioned this pull request Jan 30, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jatkinson1000 jatkinson1000 jatkinson1000 approved these changes

Assignees

@jwallwork23 jwallwork23

Labels
testing Related to FTorch testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Device error in example 3 - GPU Add 3_MultiGPU example to build process
2 participants
@jwallwork23 @jatkinson1000