Skip to content

DOC document cython typedefs #28865

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
Merged
merged 2 commits into from
Apr 23, 2024
Merged

DOC document cython typedefs #28865

merged 2 commits into from
Apr 23, 2024

Conversation

lorentzenchr
Copy link
Member

Reference Issues/PRs

Closes #25572.

What does this implement/fix? Explain your changes.

This adds a short entry in the developer guide about our central cython typedefs.

Any other comments?

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 21, 2024 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: d77bf0f. Link to the linter CI: here

@lorentzenchr lorentzenchr added the Quick Review For PRs that are quick to review label Apr 21, 2024
betatim
betatim reviewed Apr 22, 2024
View reviewed changes
betatim
betatim reviewed Apr 22, 2024
View reviewed changes
betatim
betatim approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@betatim betatim left a comment

Choose a reason for hiding this comment

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

Two small suggestions to improve the wording.

Otherwise LGTM!

Question: should we close the issue you refer to? It seems the discussion is unfinished, or has it run out of steam? Something to ponder for the person pressing merge.

@lorentzenchr @betatim
Apply suggestions from code review
d77bf0f 
Co-authored-by: Tim Head <betatim@gmail.com>
@lorentzenchr
Copy link
Member Author

It seems the discussion is unfinished, or has it run out of steam?

I checked all ocurrences of ctypedef and it seems fine to me to close the issue.

@betatim
Copy link
Member

betatim commented Apr 22, 2024 •
edited
Loading

I think the CI error isn't exactly because of this change, but it is strangely topical. This is the failure:

 Error compiling Cython file:
  ------------------------------------------------------------
  ...
  from ...utils._typedefs cimport float64_t, float32_t, int32_t, intp_t
  ^
  ------------------------------------------------------------

  sklearn/metrics/_pairwise_distances_reduction/_datasets_pair.pxd:1:0: relative cimport beyond main package is not allowed
  [74/249] Compiling Cython source sklearn/metrics/_pairwise_distances_reduction/_base.pyx
  FAILED: sklearn/metrics/_pairwise_distances_reduction/_base.cpython-312-darwin.so.p/sklearn/metrics/_pairwise_distances_reduction/_base.pyx.cpp
  cython -M --fast-fail -3 --cplus '-X language_level=3' '-X boundscheck=False' '-X wraparound=False' '-X initializedcheck=False' '-X nonecheck=False' '-X cdivision=True' '-X profile=False' --include-dir /Users/runner/work/1/s/build/cp312 sklearn/metrics/_pairwise_distances_reduction/_base.pyx -o sklearn/metrics/_pairwise_distances_reduction/_base.cpython-312-darwin.so.p/sklearn/metrics/_pairwise_distances_reduction/_base.pyx.cpp

  Error compiling Cython file:
  ------------------------------------------------------------
  ...
  from cython cimport final

  from ...utils._typedefs cimport intp_t, float64_t
  ^
  ------------------------------------------------------------

  sklearn/metrics/_pairwise_distances_reduction/_base.pxd:3:0: relative cimport beyond main package is not allowed

What do we do? It seems a fundamental "error" in the sense that if it is true we should see it in all builds, but apparently no (only macOS pylatest_conda_mkl_no_openmp fails)t? This confuses me.

@lorentzenchr
Copy link
Member Author

lorentzenchr commented Apr 22, 2024 •
edited
Loading

Maybe be should prefer absolute imports over relative ones (I somewhere read that this is recommended).
But as you mentioned. this is completely unrelated to this PR.

@jeremiedbb
Copy link
Member

jeremiedbb commented Apr 23, 2024 •
edited
Loading

Maybe #28821 missed some dependencies on _typedefs. @lesteve it seems related to build dependency ordering, but with a slightly different error, see #28865 (comment)

EDIT: I restarted the failing job and it passed this time. It's still the macos no openmp job, that's why I think it's the same kind of issue

@jeremiedbb jeremiedbb merged commit 040347a into scikit-learn:main Apr 23, 2024
30 checks passed
@lorentzenchr lorentzenchr deleted the doc_cython_types branch April 23, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@betatim betatim betatim approved these changes

Assignees
No one assigned
Labels
Documentation Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RFC Guideline for usage of Cython types
4 participants
@lorentzenchr @betatim @jeremiedbb @ogrisel