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

Fix allocation mishandling in CUDA array creation #1328

Merged
merged 10 commits into from
Apr 3, 2023
Merged

Conversation

framdani
Copy link
Member

@framdani framdani commented Feb 20, 2023

This PR fixes a bug where the allocation of the CUDA array was not being properly handled. Fixes pyccel-cuda#2.

Changes Made:

  • Fixed the allocation of the CUDA array to prevent segmentation fault.
  • Added tests to ensure proper handling of the CUDA array allocation.

@framdani framdani self-assigned this Feb 20, 2023
@framdani framdani added the Language:Ccuda stuff related to ccuda code label Feb 20, 2023
@framdani framdani marked this pull request as draft February 20, 2023 16:39
@framdani framdani marked this pull request as ready for review February 20, 2023 22:49
@framdani framdani requested a review from bauom February 20, 2023 22:49
Copy link
Member

@EmilyBourne EmilyBourne left a comment

Choose a reason for hiding this comment

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

This looks right to me. I assume there are already tests which test this part of the code? Do you have any idea what additional test might have caught this earlier (e.g. we use valgrind for ndarrays, I don't know if that is possible for cuda)?

Please can you expand the PR description to explain the changes that you made.

Copy link
Contributor

@bauom bauom left a comment

Choose a reason for hiding this comment

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

we should start populating a new test file that can run locally for now so we do not have the same problem of run time errors that slips by between commits.

can you create a file that does the same as what in the file below just for the array creation tests.
https://github.com/pyccel/pyccel/blob/master/tests/ndarrays/test_ndarrays.c

@framdani
Copy link
Member Author

framdani commented Mar 2, 2023

This looks right to me. I assume there are already tests which test this part of the code? Do you have any idea what additional test might have caught this earlier (e.g. we use valgrind for ndarrays, I don't know if that is possible for cuda)?

Please can you expand the PR description to explain the changes that you made.

There were no tests that covered this part of the code previously. I believe the tests I've added now ensure that the changes I made do not introduce any new bugs.

@EmilyBourne
Copy link
Member

@framdani The tests look good, but I think you are missing something so that they are compiled and run in the CI. Ie. the equivalent of this file: https://github.com/pyccel/pyccel/blob/master/tests/ndarrays/conftest.py and/or this file : https://github.com/pyccel/pyccel/blob/cuda_devel/.github/actions/valgrind_run/action.yml

@framdani framdani closed this Mar 2, 2023
@framdani framdani reopened this Mar 2, 2023
@framdani
Copy link
Member Author

framdani commented Mar 2, 2023

@EmilyBourne you're right , the tests I added were intended to be run locally for now. I'll do some research and ensure that the necessary configuration are added so that they can run in the CI.

@EmilyBourne
Copy link
Member

@EmilyBourne you're right , the tests I added were intended to be run locally for now. I'll do some research and ensure that the necessary configuration are added so that they can run in the CI.

@framdani Ok. No need to reinvent the wheel, I think it should be as simple as copying this file into your folder : https://github.com/pyccel/pyccel/blob/master/tests/ndarrays/conftest.py and:

@bauom
Copy link
Contributor

bauom commented Mar 2, 2023

@EmilyBourne you're right , the tests I added were intended to be run locally for now. I'll do some research and ensure that the necessary configuration are added so that they can run in the CI.

@framdani Ok. No need to reinvent the wheel, I think it should be as simple as copying this file into your folder : https://github.com/pyccel/pyccel/blob/master/tests/ndarrays/conftest.py and:

* modifying it to collect `.cu` files instead of `.c` files

* Fixing the compilation command : https://github.com/pyccel/pyccel/blob/master/tests/ndarrays/conftest.py#L52-L53

that should work but we should ignore them in GitHub action for now as we can not run them.

@EmilyBourne
Copy link
Member

EmilyBourne commented Mar 2, 2023

@EmilyBourne you're right , the tests I added were intended to be run locally for now. I'll do some research and ensure that the necessary configuration are added so that they can run in the CI.

@framdani Ok. No need to reinvent the wheel, I think it should be as simple as copying this file into your folder : https://github.com/pyccel/pyccel/blob/master/tests/ndarrays/conftest.py and:

* modifying it to collect `.cu` files instead of `.c` files

* Fixing the compilation command : https://github.com/pyccel/pyccel/blob/master/tests/ndarrays/conftest.py#L52-L53

that should work but we should ignore them in GitHub action for now as we can not run them.

Oh yes! I keep forgetting that we still can't run these tests. In that case I wonder if it's possible to split it into 2 tests?

  1. Compile with -Wall -Werror in the CI
  2. Execute (not run on CI but available for testing before merging)

Copy link
Member

@EmilyBourne EmilyBourne left a comment

Choose a reason for hiding this comment

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

It looks like there was some confusion when fixing the collisions. Please can you fix this

.github/actions/pytest_run_cuda/action.yml Outdated Show resolved Hide resolved
.github/workflows/Github_pytest.yml Outdated Show resolved Hide resolved
MANIFEST.in Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@EmilyBourne
Copy link
Member

Fixes pyccel-cuda#2 ?

@framdani framdani requested review from EmilyBourne and bauom March 22, 2023 13:25
tests/cuda_ndarrays/conftest.py Outdated Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Outdated Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Outdated Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Outdated Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Outdated Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Outdated Show resolved Hide resolved
tests/cuda_ndarrays/conftest.py Outdated Show resolved Hide resolved
tests/cuda_ndarrays/test_cuda_ndarrays.cu Outdated Show resolved Hide resolved
@EmilyBourne EmilyBourne added Ready_for_review Received at least one approval. Requires review from senior developer and removed needs_initial_review labels Mar 29, 2023
@bauom bauom added Ready_to_merge Approved by senior developer. Ready for final approval and merge and removed Ready_for_review Received at least one approval. Requires review from senior developer labels Apr 3, 2023
@bauom bauom merged commit 58ba91a into cuda_main_temp Apr 3, 2023
@bauom bauom deleted the fix-1327 branch April 3, 2023 15:07
bauom added a commit to pyccel/pyccel-cuda that referenced this pull request Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language:Ccuda stuff related to ccuda code Ready_to_merge Approved by senior developer. Ready for final approval and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in CUDA array creation due to allocation mishandling
3 participants