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.

Sign up for GitHub

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

Jump to bottom

Embed Cython docstrings in generated files. #58

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Embed Cython docstrings in generated files. #58

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions python/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ cmake_minimum_required(VERSION 3.20.1 FATAL_ERROR)

set(kvikio_version 22.06.00)

set(CYTHON_FLAGS
"--directive binding=True,embedsignature=True,always_allow_keywords=True"
Copy link
Member

Choose a reason for hiding this comment

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

Are there other things we should handle this way like opting into Python 3 syntax, using C++, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Using C++ is generally handled by Cython using file extensions, but with scikit-build CMake will take care of that so that isn't necessary.
  • We could opt into various optimizations like turning of bounds checking and wraparounds, but since we don't actually write much Cython code and it's just there for bindings I don't think it's necessary. We can always revisit if we ever start incorporating actual Cython code into performance-critical sections.
  • Opting into Python 3 syntax is definitely a possibility. I'd be open to specifying a language level here. Note that whenever we upgrade to Cython 3.0 (I think the main obstacle right now is getting try building 3.0.0 pre-releases conda-forge/cython-feedstock#75 through) the language level will automatically be set to 3, but no harm in doing it before. I don't know how big of a difference it makes given our limited Cython (Python files default to the language level of the interpreter, so those will already be 3), so let me know what you'd like me to do here.
  • We may want to add some CMake options to enable Cython profiling that would modify these flags, but that's probably something that can wait until someone has a use case?

Copy link
Member

Choose a reason for hiding this comment

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

Coming back to this comment, wondering if we should handle this as part of other needed updates to the build tooling here ( #186 )

Copy link
Contributor Author

@vyasr vyasr Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

  • I'd still want to see the benefits of turning off bounds checking etc. Also in general IMO we'd be better off using Cython's decorator syntax to do this on a per-function basis rather than globally where it is more likely to unintentionally lead to bugs.
  • Same deal for the profiling flags, not much reason to add them unless someone is actually profiling Cython IMO.
  • We recently tried opting into language_level=3 and found some issues, which I suspect may be bugs in the Cython<3 implementation of it but haven't had time to confirm. We'll probably need to revisit, but at this point I would expect the language_level transition to happen as part of a broader RAPIDS switch to using Cython 3.

CACHE STRING "The directives for Cython compilation.")

project(
kvikio-python
VERSION ${kvikio_version}
Expand Down