Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-7669

Remove GPU warping, convolution, and support code

    Details

    • Type: Story
    • Status: Done
    • Priority: Major
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Templates:
    • Story Points:
      2
    • Sprint:
      DRP S17-1
    • Team:
      Data Release Production

      Description

      Implement RFC-224.

        Issue Links

          Activity

          Hide
          swinbank John Swinbank added a comment - - edited

          Hi Perry,

          A few comments:

          First, most of what you've removed looks fine. Good job.

          However, it seems like you have not only renamed convCpuGpuShared.cc, but also copied and pasted the only function it contains into BasicConvolve.cc. The new code in BasicConvolve.cc actually contains a note that it is the same as the code in BasicConvolve.cc: that is, it asserts that it is a copied & pasted version of itself! This clearly needs to be fixed.

          I suggest that, since the "shared" in the old filenames refers to code being shared between CPU & GPU implementations, there's no longer any need to retain this convShared.cc file. Simply delete it, and the accompanying header, and migrate all the code into BasicConvolve.cc. Ensure you remove the weird comment about it being a copy & pasted version when you do so. You can put assertDimensionsOK in an anonymous namespace, since it's only referred to from within that file.

          Other than that, I see quite a few remaining references to CUDA and/or GPUs in comments & setup and build scripts:

          [jds@magpie afw (tickets/DM-7669)]$ grep -ri cuda *
          lib/SConscript:if env.ProductDir('cuda_toolkit') is not None:
          lib/SConscript:    objs.append("#src/math/detail/cudaLanczos.os")
          src/math/detail/SConscript:if env.ProductDir('cuda_toolkit') is not None:
          src/math/detail/SConscript:    envCudaCommon = env.Clone()
          src/math/detail/SConscript:    envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ')
          src/math/detail/SConscript:    envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ')
          src/math/detail/SConscript:    envConv = envCudaCommon.Clone()
          src/math/detail/SConscript:    envWarp = envCudaCommon.Clone()
          src/math/detail/SConscript:    gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu'])
          ups/afw.cfg:    "optional": ["cuda_toolkit"],
          ups/afw.table:setupOptional(cuda_toolkit)
           
          [jds@magpie afw (tickets/DM-7669)]$ grep -ri GPU * | grep -v ipynb
          include/lsst/afw/math/detail/ConvolveShared.h: * @brief CPU and GPU convolution shared code
          include/lsst/afw/math/detail/PositionFunctor.h: * @brief GPU accelerared image warping
          lib/SConscript:    objs.append("#src/math/detail/convGPU.os")
          src/math/detail/SConscript:    gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"])
          src/math/detail/SConscript:    gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu'])
          

          Please remove all of these before you merge.

          Show
          swinbank John Swinbank added a comment - - edited Hi Perry, A few comments: First, most of what you've removed looks fine. Good job. However, it seems like you have not only renamed convCpuGpuShared.cc , but also copied and pasted the only function it contains into BasicConvolve.cc . The new code in BasicConvolve.cc actually contains a note that it is the same as the code in BasicConvolve.cc : that is, it asserts that it is a copied & pasted version of itself! This clearly needs to be fixed. I suggest that, since the "shared" in the old filenames refers to code being shared between CPU & GPU implementations, there's no longer any need to retain this convShared.cc file. Simply delete it, and the accompanying header, and migrate all the code into BasicConvolve.cc . Ensure you remove the weird comment about it being a copy & pasted version when you do so. You can put assertDimensionsOK in an anonymous namespace, since it's only referred to from within that file. Other than that, I see quite a few remaining references to CUDA and/or GPUs in comments & setup and build scripts: [jds@magpie afw (tickets/DM-7669)]$ grep -ri cuda * lib/SConscript:if env.ProductDir('cuda_toolkit') is not None: lib/SConscript: objs.append("#src/math/detail/cudaLanczos.os") src/math/detail/SConscript:if env.ProductDir('cuda_toolkit') is not None: src/math/detail/SConscript: envCudaCommon = env.Clone() src/math/detail/SConscript: envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ') src/math/detail/SConscript: envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ') src/math/detail/SConscript: envConv = envCudaCommon.Clone() src/math/detail/SConscript: envWarp = envCudaCommon.Clone() src/math/detail/SConscript: gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) ups/afw.cfg: "optional": ["cuda_toolkit"], ups/afw.table:setupOptional(cuda_toolkit)   [jds@magpie afw (tickets/DM-7669)]$ grep -ri GPU * | grep -v ipynb include/lsst/afw/math/detail/ConvolveShared.h: * @brief CPU and GPU convolution shared code include/lsst/afw/math/detail/PositionFunctor.h: * @brief GPU accelerared image warping lib/SConscript: objs.append("#src/math/detail/convGPU.os") src/math/detail/SConscript: gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"]) src/math/detail/SConscript: gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) Please remove all of these before you merge.
          Hide
          pgee Perry Gee added a comment -

          John Swinbank I didn't mean to checkin this change to BasicConvolve – I started to move the one routine there, then decided that I wouldn't move anything until I tried to get an opinion during review. I think that the detail directory is still oddly organized, with more modules than are really needed. However, I will make the change as suggested.

          Show
          pgee Perry Gee added a comment - John Swinbank I didn't mean to checkin this change to BasicConvolve – I started to move the one routine there, then decided that I wouldn't move anything until I tried to get an opinion during review. I think that the detail directory is still oddly organized, with more modules than are really needed. However, I will make the change as suggested.
          Hide
          swinbank John Swinbank added a comment -

          Hey Perry Gee: please remember to push your rebased ticket branch to github before you merge it with master. Otherwise, GitHub doesn't realise that it can close the PR.

          Currently, your ticket branch seems to contain a bunch of weird obsolete commits, while the merge to master came from a user branch. That's unfortunate!

          That minor complaint aside: thanks for getting this done!

          Show
          swinbank John Swinbank added a comment - Hey Perry Gee : please remember to push your rebased ticket branch to github before you merge it with master. Otherwise, GitHub doesn't realise that it can close the PR. Currently, your ticket branch seems to contain a bunch of weird obsolete commits, while the merge to master came from a user branch. That's unfortunate! That minor complaint aside: thanks for getting this done!
          Hide
          pgee Perry Gee added a comment -

          There are times when git does things I don't understand and can't fix. Right before I checked in, I did a pull, a git rebase master on the branch, and then a git merge --noff branch. I was left with a state where there were 19 commits on one and 20 on the other which differed. Since I could not figure out how to fix the problem, I started over on a different branch, did a diff of the final result, then merged the second branch to master.

          Show
          pgee Perry Gee added a comment - There are times when git does things I don't understand and can't fix. Right before I checked in, I did a pull, a git rebase master on the branch, and then a git merge --noff branch. I was left with a state where there were 19 commits on one and 20 on the other which differed. Since I could not figure out how to fix the problem, I started over on a different branch, did a diff of the final result, then merged the second branch to master.
          Hide
          swinbank John Swinbank added a comment -

          There are times when git does things I don't understand and can't fix.

          Please don't hesitate to ask for help when this happens: I'd rather we spend a few extra minutes to get things right.

          Show
          swinbank John Swinbank added a comment - There are times when git does things I don't understand and can't fix. Please don't hesitate to ask for help when this happens: I'd rather we spend a few extra minutes to get things right.

            People

            • Assignee:
              pgee Perry Gee
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              John Swinbank
              Watchers:
              Colin Slater, Jim Bosch, John Swinbank, Perry Gee
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development

                  Agile