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

Moving device functions to cuh files and deprecating hpp #524

Merged

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Feb 22, 2022

For consistency, we had originally scraped through the primitive functions and used the hpp extension across the public API. However, it was brought to my attention more recently that this is confusing when considering the larger scope of the project- which also contains many host-only APIs that don't require a cuda-enabled compiler.

However, as we're gaining more consumers, we need to start being more careful about making breaking changes to the public APIs and their header files. For this reason, I'm opting to copy the existing hpp files into cuh files, deprecating the hpp files, and using #define w/ conditionals to make sure the contents from only one file get defined even if both are included (for example, when a user includes filea.hpp but raft internally includes filea.cuh. This should allow us to set a version where we can make an announcement to remove the offending hpp files and give ample notice before the breaking change is made.

@cjnolet cjnolet added the improvement Improvement / enhancement to an existing function label Feb 22, 2022
@cjnolet cjnolet added this to PR-WIP in RAFT v22.04 Release via automation Feb 22, 2022
@cjnolet cjnolet requested a review from a team as a code owner February 22, 2022 16:08
@cjnolet cjnolet added the non-breaking Non-breaking change label Feb 22, 2022
@github-actions github-actions bot added the cpp label Feb 22, 2022
@cjnolet
Copy link
Member Author

cjnolet commented Feb 22, 2022

In preparation for when we do end up removing the deprecated hpp files, I've written a script to automatically update includes of downstream repositories so they match the extensions in RAFT.

import sys
import glob
import re
import shutil

if __name__ == "__main__":

    script, raft_include_path, source_path = sys.argv

    p = re.compile(r'.*\#include \<(.*)\.(.*)\>')

    header_map = {}
    files_globbed = []

    types = ("/**/*.cuh", "/**/*.hpp", "/**/*.h")

    for files in types:
      files_globbed.extend([f for f in glob.glob(raft_include_path + files, recursive=True)])

    for f in files_globbed:
      fname, fext = f.split(".")
      header_map[fname.replace(raft_include_path, "")] = fext

    types_to_correct = ("/**/*.cuh", "/**/*.hpp", "/**/*.h", "/**/*.cu", "/**/*.cpp", "/**/*.c")

    files_to_correct = []
    for files in types_to_correct:
      files_to_correct.extend([f for f in glob.glob(source_path + files, recursive=True)])

    for f in files_to_correct:
        
      with open(f, 'r') as fp:
        with open("__tmp", "w") as tfp:
          lines = fp.readlines()
          for line in lines:
            match_br = p.match(line)

            if match_br is None:
                tfp.write(line)
            else:
              fname = match_br.group(1)
              fext = match_br.group(2)

              if fname in header_map and header_map[fname] != fext:
                  replacement = "#include <%s.%s>" % (fname, header_map[fname])
                  new_line = p.sub(replacement, line)
                  tfp.write(new_line)
                  print("fname=%s, fext=%s, header_map=%s, new=%s" % (fname, fext, header_map[fname], new_line))
              else:
                  tfp.write(line)

      shutil.move("__tmp", f)

Invoke the script w/ 2 arguments: python match_raft_headers.py <raft_include_path> <consuming_repo_source_path>

@cjnolet cjnolet moved this from PR-WIP to PR-Needs review in RAFT v22.04 Release Feb 23, 2022
@cjnolet cjnolet changed the title Lots of renames to denote device-specific APIs Moving device functions to cuh files and deprecating hpp Feb 24, 2022
@cjnolet
Copy link
Member Author

cjnolet commented Feb 24, 2022

rerun tests

Copy link
Contributor

@msadang msadang left a comment

Choose a reason for hiding this comment

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

All checks have passed. Numerous small changes of year and file extension make up the majority of the changes. Looks good to me.

RAFT v22.04 Release automation moved this from PR-Needs review to PR-Reviewer approved Feb 25, 2022
@cjnolet
Copy link
Member Author

cjnolet commented Feb 25, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 08003c2 into rapidsai:branch-22.04 Feb 25, 2022
RAFT v22.04 Release automation moved this from PR-Reviewer approved to Done Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants