Skip to content

Remove cython from installation #58

Merged
erezsh merged 5 commits intolark-parser:masterfrom
tiagocoutinho:cython-build-only
Jul 15, 2025
Merged

Remove cython from installation #58
erezsh merged 5 commits intolark-parser:masterfrom
tiagocoutinho:cython-build-only

Conversation

@tiagocoutinho
Copy link
Contributor

Cython should only be necessary during build.
Installations should stay cython free (current version of cython alone takes 20MB)

Real change was adding cython as build dependency on pyproject.toml
def cythonize(*args, **kwargs):
from Cython.Build import cythonize

return cythonize(*args, **kwargs)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not doing anything that fixes installation without cython installed.
The real fix was on pyproject.toml by adding cython as build dependency

@Erotemic
Copy link
Collaborator

Erotemic commented Jul 4, 2025

I think this is correct.

@erezsh
Copy link
Member

erezsh commented Jul 6, 2025

I wonder what it means. Maybe the version needs to be bumped?

@Erotemic
Copy link
Collaborator

Erotemic commented Jul 6, 2025

Maybe the error is a CI hickup and needs to be rerun? Or by updating to actions/upload-artifact@v4, maybe its doing a duplicate name check that was previously silently overwriting data?

@tiagocoutinho
Copy link
Contributor Author

I think you're right @Erotemic.
I had a look at update-artifact@v4 changes.
Looks like they added a merge artifact sub-action.
I've updated the PR with what I think are the needed changes

@erezsh erezsh requested a review from Copilot July 7, 2025 15:33
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes Cython from runtime installation requirements and ensures it’s only used at build time, and it updates CI workflow steps to use the latest artifact action versions and revised wheel-building patterns.

  • Always import cythonize and drop Cython from install_requires
  • Restrict Cython to setup_requires
  • Bump actions/upload/download-artifact to v4 and adjust wheel-skip patterns

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
setup.py Removed fallback import and Cython from install_requires, now only in setup_requires
.github/workflows/tests.yml Updated upload/download artifact actions to v4, renamed sdist artifact, and refined skip patterns
Comments suppressed due to low confidence (3)

.github/workflows/tests.yml:47

  • [nitpick] The artifact name "wheels-src" is misleading for a source distribution; consider renaming it to "sdist" or "source-dist" for better clarity.
        name: wheels-src

.github/workflows/tests.yml:135

  • This download step only fetches merged wheel artifacts; the sdist artifact ("wheels-src") isn’t downloaded, so the source distribution won’t be available downstream. Add a separate download-artifact step for the sdist.
      uses: actions/download-artifact@v4

setup.py:33

  • The use of setup_requires is deprecated by modern pip; consider moving the Cython build requirement into a pyproject.toml under [build-system] as recommended by PEP 518.
    setup_requires=["Cython>=3.0,<3.1"],

@tiagocoutinho
Copy link
Contributor Author

Please let me know if you need anything else from my side

@erezsh erezsh merged commit 2ad4b3a into lark-parser:master Jul 15, 2025
11 checks passed
@erezsh
Copy link
Member

erezsh commented Jul 15, 2025

Thanks for the PR!

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