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

GH-34564: [Python][C++] Update code to compile with cython 3 #34726

Merged

Conversation

westonpace
Copy link
Member

@westonpace westonpace commented Mar 24, 2023

Rationale for this change

Cython 3 has some breaking changes:

  • && is no longer supported
  • nogil must appear at the end of the line

In addition we had to work around a bug: cython/cython#5333

What changes are included in this PR?

Minor changes to the cython code.

Are these changes tested?

No, and they probably should be at some point. However, Cython 3 has not yet released and so it would be impractical to update our CI to run Cython 3 until that happens.

Are there any user-facing changes?

No

@github-actions
Copy link

@github-actions
Copy link

⚠️ GitHub issue #34564 has been automatically assigned in GitHub to PR creator.

@westonpace westonpace changed the title GH-34564: [C++] Update code to compile with cython 3 GH-34564: [Python][C++] Update code to compile with cython 3 Mar 24, 2023
@github-actions github-actions bot added the awaiting committer review Awaiting committer review label Mar 24, 2023
@lidavidm
Copy link
Member

We could add a nightly build to catch Cython 3 issues?

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Mar 24, 2023
@westonpace
Copy link
Member Author

We could add a nightly build to catch Cython 3 issues?

Eventually, yes, I agree. At the moment Cython 3 hasn't released yet. The beta will not build due to known errors in the beta. I had to compile cython from the latest commit to get this to work. That's probably not worth maintaining so we can just wait and put the nightly build in (or update the existing builds) when Cython 3 is released.

@westonpace westonpace merged commit c8e0096 into apache:main Mar 24, 2023
@da-woods
Copy link

A couple of quick comments:

  • && is no longer supported
  • nogil must appear at the end of the line

I think these are both warnings (so don't have to be breaking changes).

&& was never really supported. It just got interpreted as "reference to reference". Now we understand it in a few limited contexts, so issue a warning for the other contexts.

@bdice
Copy link

bdice commented Mar 24, 2023

Thanks for this fix @westonpace!

@ursabot
Copy link

ursabot commented Mar 25, 2023

Benchmark runs are scheduled for baseline = e7d6c13 and contender = c8e0096. c8e0096 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.24% ⬆️0.09%] test-mac-arm
[Finished ⬇️0.51% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.15% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] c8e00969 ec2-t3-xlarge-us-east-2
[Finished] c8e00969 test-mac-arm
[Finished] c8e00969 ursa-i9-9960x
[Finished] c8e00969 ursa-thinkcentre-m75q
[Finished] e7d6c13d ec2-t3-xlarge-us-east-2
[Finished] e7d6c13d test-mac-arm
[Finished] e7d6c13d ursa-i9-9960x
[Finished] e7d6c13d ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Mar 25, 2023

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

rtpsw pushed a commit to rtpsw/arrow that referenced this pull request Mar 27, 2023
…pache#34726)

### Rationale for this change

Cython 3 has some breaking changes:

 * `&&` is no longer supported
 * `nogil` must appear at the end of the line

In addition we had to work around a bug: cython/cython#5333

### What changes are included in this PR?

Minor changes to the cython code.

### Are these changes tested?

No, and they probably should be at some point.  However, Cython 3 has not yet released and so it would be impractical to update our CI to run Cython 3 until that happens.

### Are there any user-facing changes?

No
* Closes: apache#34564

Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
@jorisvandenbossche
Copy link
Member

The beta will not build due to known errors in the beta. I had to compile cython from the latest commit to get this to work.

@westonpace I see that a second beta was released yesterday. Should that in principle be sufficient? (not requiring to compile latest cython manually)

ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
…pache#34726)

### Rationale for this change

Cython 3 has some breaking changes:

 * `&&` is no longer supported
 * `nogil` must appear at the end of the line

In addition we had to work around a bug: cython/cython#5333

### What changes are included in this PR?

Minor changes to the cython code.

### Are these changes tested?

No, and they probably should be at some point.  However, Cython 3 has not yet released and so it would be impractical to update our CI to run Cython 3 until that happens.

### Are there any user-facing changes?

No
* Closes: apache#34564

Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
pitrou pushed a commit that referenced this pull request Jul 18, 2023
…uild dependencies (#36743)

### Rationale for this change

Although we already fixed some cython 3 build issues (#34726), some new have been introduced, which we are seeing now cython 3 is released (#36730)

Adding an upper pin (<3) for the release, so we have more time (the full 14.0 release cycle) to iron out issues.
* Closes: #36744

Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
raulcd pushed a commit that referenced this pull request Jul 18, 2023
…uild dependencies (#36743)

### Rationale for this change

Although we already fixed some cython 3 build issues (#34726), some new have been introduced, which we are seeing now cython 3 is released (#36730)

Adding an upper pin (<3) for the release, so we have more time (the full 14.0 release cycle) to iron out issues.
* Closes: #36744

Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
chelseajonesr pushed a commit to chelseajonesr/arrow that referenced this pull request Jul 20, 2023
…rrow build dependencies (apache#36743)

### Rationale for this change

Although we already fixed some cython 3 build issues (apache#34726), some new have been introduced, which we are seeing now cython 3 is released (apache#36730)

Adding an upper pin (<3) for the release, so we have more time (the full 14.0 release cycle) to iron out issues.
* Closes: apache#36744

Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this pull request Aug 20, 2023
…rrow build dependencies (apache#36743)

### Rationale for this change

Although we already fixed some cython 3 build issues (apache#34726), some new have been introduced, which we are seeing now cython 3 is released (apache#36730)

Adding an upper pin (<3) for the release, so we have more time (the full 14.0 release cycle) to iron out issues.
* Closes: apache#36744

Authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Python] Building with Cython 3 shows warnings
6 participants