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 MaxRecursionDepth error, Memory leaks and move to CUDA12 #174

Merged
merged 45 commits into from
Aug 8, 2024

Conversation

chaithyagr
Copy link
Member

@chaithyagr chaithyagr commented Aug 5, 2024

We had some issues of MaxRecurssionDepth, which was fixed and on extensively testing, I saw a memory leak in gpuNUFFT (fixed in chaithyagr/gpuNUFFT#23). Some more memory leaks from cuFFT comes from from CUDA11.8: https://forums.developer.nvidia.com/t/nvc-cufft-memory-leak/279476/3

To fix this, we moved to using CUDA12.1, which is default version which comes with torch.

Also have a bunch of other miscellaneous tasks:

  1. Added support for binder button in docs.
  2. Fixed a minor bug in smaps estimation which didnt allow int values for blurr_factor.

@chaithyagr chaithyagr requested a review from paquiteau August 5, 2024 09:02
@chaithyagr chaithyagr marked this pull request as draft August 5, 2024 09:03
@chaithyagr chaithyagr self-assigned this Aug 5, 2024
@chaithyagr chaithyagr marked this pull request as ready for review August 5, 2024 09:20
@chaithyagr chaithyagr requested review from alineyyy and removed request for alineyyy August 5, 2024 09:21
Copy link
Member

@paquiteau paquiteau left a comment

Choose a reason for hiding this comment

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

Great Work ! moving to cuda12 is really nice. We should probably make a release after this.

I have some minor comments/suggestions to address before that though ;)

@@ -282,7 +280,7 @@ jobs:
BuildDocs:
name: Build API Documentation
runs-on: gpu
if: ${{ (contains(github.event.head_commit.message, 'docs_build')) || (github.ref == 'refs/heads/master')}}
if: ${{ !(contains(github.event.head_commit.message, 'docs_build')) || (github.ref == 'refs/heads/master') }}
Copy link
Member

Choose a reason for hiding this comment

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

so whats the expected behavior ? one needs to put docs_build to skip the building of the documentation ? seems weird to me...

Copy link
Member Author

Choose a reason for hiding this comment

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

Its weird, when I added this, it is building .. I am lost but it works :P

Copy link
Member

Choose a reason for hiding this comment

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

Let's clear this then, We should build the doc:

  • In PR to master, when the docs_build keyword is present.
  • On every commit to master (aka after merge)

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/contexts#github-context

I think we should use github.head_ref (and the contains)

Copy link
Member Author

Choose a reason for hiding this comment

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

The ref aspect is fine. The containts part doesnt work. I am trying to understand whats broken. For some reason it does not trigger even when I use docs_build

Copy link
Member Author

@chaithyagr chaithyagr Aug 6, 2024

Choose a reason for hiding this comment

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

Okay this is broken. For now, we dont check docs_build and also style. (Cause github.event.head_commit is empty if the event is triggered with pull_request I will open an issue (#176) to fix this, but its not easy :P
We can proceed with current PR for now

src/mrinufft/extras/smaps.py Outdated Show resolved Hide resolved
src/mrinufft/operators/base.py Outdated Show resolved Hide resolved
@chaithyagr chaithyagr changed the title Fix MaxRecursionDepth, Memory leaks and move to CUDA12 Fix MaxRecursionDepth error, Memory leaks and move to CUDA12 Aug 8, 2024
@chaithyagr chaithyagr requested a review from paquiteau August 8, 2024 06:38
@chaithyagr chaithyagr merged commit 87ffecf into mind-inria:master Aug 8, 2024
11 checks passed
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.

2 participants