Details

    • Type: Technical task
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None
    • Team:
      SQuaRE

      Description

      After manually removing the product dependencies, sconsUtils still attempts to import eups.

      $ AFW_DIR=. scons 
      scons: Reading SConscript files ...
      Unable to import eups; guessing flavor
      Checking who built the CC compiler...error: no result
      CC is gcc version 4.8.3
      Checking for C++11 support
      Checking whether the C++ compiler worksyes
      C++11 supported with '-std=c++11'
      Checking for C++ header file tr1/unordered_map... yes
      Setting up environment to build package 'afw'.
      Unable to import eups; guessing flavor
      Could not find EUPS product dir for 'afw'; using /home/vagrant/afw.
      Doxygen is not setup; skipping documentation build.
      ImportError: No module named eups:
        File "/home/vagrant/afw/SConstruct", line 3:
          scripts.BasicSConstruct("afw")
        File "/home/vagrant/foo/python/lsst/sconsUtils/scripts.py", line 56:
          versionModuleName, noCfgFile=noCfgFile)
        File "/home/vagrant/foo/python/lsst/sconsUtils/scripts.py", line 106:
          SCons.Script.SConscript(os.path.join(root, "SConscript"))
        File "/usr/lib/scons/SCons/Script/SConscript.py", line 609:
          return method(*args, **kw)
        File "/usr/lib/scons/SCons/Script/SConscript.py", line 546:
          return _SConscript(self.fs, *files, **subst_kw)
        File "/usr/lib/scons/SCons/Script/SConscript.py", line 260:
          exec _file_ in call_stack[-1].globals
        File "/home/vagrant/afw/lib/SConscript", line 6:
          import eups
      

        Attachments

          Issue Links

            Activity

            Hide
            jhoblitt Joshua Hoblitt added a comment -

            That was a poor description, the offender is `lib/SConscript`.

            https://github.com/lsst/afw/blob/master/lib/SConscript#L6-L9

            eups is also imported by `src/math/detail/SConscript`

            https://github.com/lsst/afw/blob/master/src/math/detail/SConscript#L4-L16

            Show
            jhoblitt Joshua Hoblitt added a comment - That was a poor description, the offender is `lib/SConscript`. https://github.com/lsst/afw/blob/master/lib/SConscript#L6-L9 eups is also imported by `src/math/detail/SConscript` https://github.com/lsst/afw/blob/master/src/math/detail/SConscript#L4-L16
            Hide
            rhl Robert Lupton added a comment -

            As Tim Jenness said in one of these tickets, this is using eups.productDir and Russell just provided a routine in utils to avoid this pattern. So please assign yourself these "no build without eups" tickets and fix them. I agree that it's a good idea to remove all use of eups to probe which packages are setup.

            Show
            rhl Robert Lupton added a comment - As Tim Jenness said in one of these tickets, this is using eups.productDir and Russell just provided a routine in utils to avoid this pattern. So please assign yourself these "no build without eups" tickets and fix them. I agree that it's a good idea to remove all use of eups to probe which packages are setup.
            Hide
            rowen Russell Owen added a comment -

            I asked Josh and he put the code changes in tickets/DM-2636

            Show
            rowen Russell Owen added a comment - I asked Josh and he put the code changes in tickets/ DM-2636
            Hide
            rowen Russell Owen added a comment -

            Review of DM-2771 (work is on tickest/DM-2636)

            For such a small change I'm afraid I have rather a lot to say.

            math/detail/SConscript:

            • I feel your code swallows too many potential errors. I suggest putting only the getPackageDir in the try clause so you do not hide errors in the rest of the code. This looks like a good use of try/else to me; see example below.
            • Picky point, but using "True" instead of "pass" in the except clause is unusual; it works but I find it confusing
            • Using getPackageDir(packageName) to determine if a package is available is clearly a bit clumsy (the old behavior of returning None meant that one function could do two different jobs, but required a lot of wrappers when you wanted an instant failure if the package was not setup, and does not translate well to C++). Since you have to do it in several places I strongly suggest adding a new function to utils: isPackageAvailable(packageName). (The name could also be isPackageSetup, but that is a very EUPS-specific way to express the question).

            Here is my suggested alternative:

            if lsst.utils.isPackageAvailable('cuda_toolkit'):
                envCudaCommon = env.Clone()
                envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ')
                envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ')
                envConv = envCudaCommon.Clone()
                envConv.Append(NVCCFLAGS = ' -maxrregcount=60 ')
                envConv.Append(NVCCFLAGS = ' --ptxas-options=-v ')
                gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"])
                envWarp = envCudaCommon.Clone()
                envWarp.Append(NVCCFLAGS = ' -maxrregcount=60 ')
                envWarp.Append(NVCCFLAGS = ' --ptxas-options=-v ')
                gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu'])
            

            Or without isPackageAvailable:

            import lsst.utils
            try:
                lsst.utils.getPackageDir('cuda_toolkit')
            except Exception:
                pass
            else:
                envCudaCommon = env.Clone()
                envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ')
                envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ')
                envConv = envCudaCommon.Clone()
                envConv.Append(NVCCFLAGS = ' -maxrregcount=60 ')
                envConv.Append(NVCCFLAGS = ' --ptxas-options=-v ')
                gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"])
                envWarp = envCudaCommon.Clone()
                envWarp.Append(NVCCFLAGS = ' -maxrregcount=60 ')
                envWarp.Append(NVCCFLAGS = ' --ptxas-options=-v ')
                gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu'])
            

            tests/SConscript:

            • The same issue applies about hiding too many errors.
            • Do you need to set $AFWDATA_DIR? If so, a better comment about why would be appreciated (though I realize this is pre-existing code). If you are not sure it is needed please try without.

            afw/lib/SConscript:

            • The same issue applies about hiding too many errors.
            Show
            rowen Russell Owen added a comment - Review of DM-2771 (work is on tickest/ DM-2636 ) For such a small change I'm afraid I have rather a lot to say. math/detail/SConscript: I feel your code swallows too many potential errors. I suggest putting only the getPackageDir in the try clause so you do not hide errors in the rest of the code. This looks like a good use of try/else to me; see example below. Picky point, but using "True" instead of "pass" in the except clause is unusual; it works but I find it confusing Using getPackageDir(packageName) to determine if a package is available is clearly a bit clumsy (the old behavior of returning None meant that one function could do two different jobs, but required a lot of wrappers when you wanted an instant failure if the package was not setup, and does not translate well to C++). Since you have to do it in several places I strongly suggest adding a new function to utils: isPackageAvailable(packageName). (The name could also be isPackageSetup, but that is a very EUPS-specific way to express the question). Here is my suggested alternative: if lsst.utils.isPackageAvailable('cuda_toolkit'): envCudaCommon = env.Clone() envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ') envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ') envConv = envCudaCommon.Clone() envConv.Append(NVCCFLAGS = ' -maxrregcount=60 ') envConv.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"]) envWarp = envCudaCommon.Clone() envWarp.Append(NVCCFLAGS = ' -maxrregcount=60 ') envWarp.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) Or without isPackageAvailable: import lsst.utils try: lsst.utils.getPackageDir('cuda_toolkit') except Exception: pass else: envCudaCommon = env.Clone() envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_13,code=\\"sm_13,compute_13\\" ') envCudaCommon.Append(NVCCFLAGS = ' -gencode=arch=compute_20,code=\\"sm_20,compute_20\\" ') envConv = envCudaCommon.Clone() envConv.Append(NVCCFLAGS = ' -maxrregcount=60 ') envConv.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envConv.SharedObject("#/src/math/detail/convGPU", ["#src/math/detail/convGPU.cu"]) envWarp = envCudaCommon.Clone() envWarp.Append(NVCCFLAGS = ' -maxrregcount=60 ') envWarp.Append(NVCCFLAGS = ' --ptxas-options=-v ') gpuObjConvolution = envWarp.SharedObject('#src/math/detail/cudaLanczos', ['#src/math/detail/cudaLanczos.cu']) tests/SConscript: The same issue applies about hiding too many errors. Do you need to set $AFWDATA_DIR? If so, a better comment about why would be appreciated (though I realize this is pre-existing code). If you are not sure it is needed please try without. afw/lib/SConscript: The same issue applies about hiding too many errors.
            Hide
            jhoblitt Joshua Hoblitt added a comment -

            I wasn't aware of the try/else construct. I'll rework the PR shortly.

            Show
            jhoblitt Joshua Hoblitt added a comment - I wasn't aware of the try/else construct. I'll rework the PR shortly.
            Hide
            jhoblitt Joshua Hoblitt added a comment -

            Russell Owen I've decided not to add another utility function to determine if a package is present. I'm concerned that it will end up being more plumbing to refactor in the near future.

            I don't know why AFWDATA_DIR is being setup in testsSConscript, I assume one of the test scripts depend on it.

            I think I've addressed your concerns about gratuitous exception hiding and repushed the branch.

            Show
            jhoblitt Joshua Hoblitt added a comment - Russell Owen I've decided not to add another utility function to determine if a package is present. I'm concerned that it will end up being more plumbing to refactor in the near future. I don't know why AFWDATA_DIR is being setup in testsSConscript, I assume one of the test scripts depend on it. I think I've addressed your concerns about gratuitous exception hiding and repushed the branch.

              People

              • Assignee:
                jhoblitt Joshua Hoblitt
                Reporter:
                jhoblitt Joshua Hoblitt
                Reviewers:
                Russell Owen, Tim Jenness
                Watchers:
                Frossie Economou, Joshua Hoblitt, Robert Lupton, Russell Owen, Tim Jenness
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: