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

Allow compatibility with Cython 3.x #10

Closed
wants to merge 4 commits into from
Closed

Allow compatibility with Cython 3.x #10

wants to merge 4 commits into from

Conversation

Marion-Artigaut
Copy link

@Marion-Artigaut
Copy link
Author

The current fix works with Cython 3.0.0 in GitHub actions with the following code:

name: CI testing 

on:
  push:
  pull_request:

jobs:
  run-tests:
    strategy:
      matrix:
        os: [ubuntu-latest]
        python-version: ['3.x']

    name: Test
    runs-on: ${{matrix.os}}

    steps:
      - name: Checkout code
        uses: actions/checkout@v3

      - name: Set up Python 
        uses: actions/setup-python@v4
        with:
          python version: ${{ matrix.python-version }}

      - name: Install dependencies
        run: |
          python -m pip install numpy matplotlib scipy python-igraph
          python -m pip install --upgrade pytest 
          python -m pip install triangle
          python -m pip install cython==3.0.0

      - name: Clone PyTetgen 
        run: |
          git clone https://github.com/Marcello-Sega/pytetgen.git
          
      - name: Modify PyTetgen
        run: |
          cd pytetgen
          sed --in-place '1s/^/#!python\n# cython: language_level=2\n/' pytetgen/pytetgen.pyx
          
      - name: Build PyTetgen
        run: |
          cd pytetgen
          python setup.py build_ext --inplace

      - name: Set pythonpath with pwd and pytetgen
        run: |
          echo "PYTHONPATH=$(pwd):$(pwd)/pytetgen" >> $GITHUB_ENV

      - name: Run tests 
        run: pytest 

@Marcello-Sega
Copy link
Owner

Dear Marion, thanks for opening the issue and for the pull request. I just wondered if it weren't better to simply go for integer division with // and keep python3 semantics in cython? What's your opinion?

@szhorvat
Copy link

szhorvat commented Oct 2, 2023

- name: Install dependencies
        run: |
          python -m pip install numpy matplotlib scipy python-igraph

Please do not use pip install python-igraph. Simply use pip install igraph. See igraph/python-igraph#699

@Marion-Artigaut
Copy link
Author

Marion-Artigaut commented Oct 10, 2023

Dear Marcello,
I have not checked the remainder of the code to see if any other errors were raised with python3 semantics. It would I agree be better to do a full update. I am not as familiar with all the ins and outs of the code like you. Would you expect more work for migration to v3.x of cython?
See documentation: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html

@Marcello-Sega
Copy link
Owner

@Marion-Artigaut could you have a look if #11 fixes it for you as well ?

@Marcello-Sega
Copy link
Owner

Fixed by #11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants