Embed Cython docstrings in generated files. #58
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vyasr
added
bug
madsbk
approved these changes
Apr 7, 2022
jakirkham
reviewed
Apr 7, 2022
| @@ -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" | |||
There was a problem hiding this comment.
Are there other things we should handle this way like opting into Python 3 syntax, using C++, etc.?
There was a problem hiding this comment.
- 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?
There was a problem hiding this comment.
Coming back to this comment, wondering if we should handle this as part of other needed updates to the build tooling here ( #186 )
There was a problem hiding this comment.
- 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=3and 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.
|
@gpucibot merge |
vuule
pushed a commit
to vuule/kvikio
that referenced
this pull request
Nov 8, 2023
…n't exist (rapidsai#58) Fixes: rapidsai/cudf#13860 This PR fixes an issue in Series.setitem where if the index is a RangeIndex, we are corrupting the series data & index object by not properly handling RangeIndex separately and not making a copy. Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR embeds signatures in Cython functions and classes so that they'll be accessible for normal Python help. While docstrings are currently present as expected, we still have to provide Cython with the appropriate directives to tell it that we want signatures.