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

Use cython distutils directives #2509

Merged
merged 21 commits into from
Jul 2, 2020

Conversation

matthewturk
Copy link
Member

This is the start of a process to make all the modules use cythonize and distutils directives. In the first PR, it just converts yt/geometry/.

Putting the configuration information into the files themselves should make it easier to update them. But, if we decide this isn't what we want, then it isn't what we want!

At present this is not working for in-place builds, which I'm hoping to address soon. Basically, the question before us is: would this be an improvement for developers' lives? I think all of the conditional compilations can be handled in this way.

@matthewturk
Copy link
Member Author

So I think this is ready to be looked at. I know that the conditional compiles need to be addressed (just pyembree; gperftools is addressed in #2510 ) and that right now the parallelized build has to be turned back on, but otherwise this should be OK.

@matthewturk matthewturk changed the title [WIP] Use cython distutils directives Use cython distutils directives Mar 30, 2020
@matthewturk
Copy link
Member Author

I've ticked off the boxes for conditional compilation of embree, and I've reverted to using the overridden built_ext function, so I think this is ready for review and maybe being included. I've tested the results of sdist and they seem to be consistent, as do the results of building in-place and via installation.

@matthewturk
Copy link
Member Author

I should note that for the purposes of review, nearly 100% of the changes that are meaningful happened in setup.py. I moved some embree stuff around so that it wouldn't get caught up in the glob patterns and I added the appropriate # distutils: directives to the top of all the cython files that needed them.

@munkm
Copy link
Member

munkm commented Mar 31, 2020

I'm happy to review this but it's going to be a week or so until after the 3.6 release. I want to comment about it here so you know it's not being forgotten! 🙂

@matthewturk
Copy link
Member Author

I'm hoping that once this is in, we can do header-level cython directives instead of function-level, which should make debugging easier as we can toggle them on/off from the command line instead of modifying individual functions. This script implements that.

import glob
prepend = "# cython: language_level=2, bounds_check=False, wraparound=False\n# cython: cdivision=True, initializedcheck=False\n"

for fn in sorted(glob.glob("yt/**/*.pyx", recursive=True)):
    print(f"Prepending for {fn}")
    lines = []
    with open(fn, "r") as f:
        for line in f:
          if line.strip().startswith("@cython"): continue
          lines.append(line)
    with open(fn, "w") as f:
        f.write(prepend)
        f.writelines(lines)

@matthewturk
Copy link
Member Author

I've merged with upstream post-#2513 to run with the new tests.

@matthewturk matthewturk added the infrastructure Related to CI, versioning, websites, organizational issues, etc label Apr 17, 2020
@Xarthisius
Copy link
Member

--- a/setup.py
+++ b/setup.py
@@ -42,7 +42,7 @@ with open('README.md') as file:
 if check_for_openmp() is True:
     omp_args = ['-fopenmp']
 else:
-    omp_args = None
+    omp_args = []
 
 if os.name == "nt":
     std_libs = []

did the trick for me on OSX.

Copy link
Member

@munkm munkm 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 good to me. It looks like there is also some mesh traversal stuff in the test install script:

https://github.com/yt-project/yt/blob/yt-4.0/tests/test_install_script.py#L39

and I think also the gitignore might need to be updated? https://github.com/yt-project/yt/blob/yt-4.0/.gitignore#L61

@munkm munkm changed the base branch from yt-4.0 to master June 22, 2020 15:23
@matthewturk
Copy link
Member Author

Had to fix the conflicts; ptal

@matthewturk
Copy link
Member Author

@jzuhone can you check that I've correctly ported your std=c++11 here?

@munkm munkm merged commit ae20cf0 into yt-project:master Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Related to CI, versioning, websites, organizational issues, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants