Skip to content

Migrate random to pyproject.toml from setup.py #62

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

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

ndgrigorian
Copy link
Collaborator

@ndgrigorian ndgrigorian commented Apr 13, 2025

This PR migrates mkl_random to pyproject.toml from setup.py

Also adjusts build system, removing mkl-service as a test requirement (it was previously added as part of a work-around, which was removed) and moves to use python -m pip install in build scripts instead of calling setup.py and updates min NumPy version

@ndgrigorian ndgrigorian force-pushed the migrate-random-to-pyproject-toml branch from 14569bf to 057f53c Compare April 13, 2025 08:01
@ndgrigorian ndgrigorian force-pushed the migrate-random-to-pyproject-toml branch from 057f53c to 091f047 Compare April 13, 2025 18:24
@ndgrigorian
Copy link
Collaborator Author

ndgrigorian commented Apr 13, 2025

@vtavana
For some reason, including the project.license field in pyproject.toml causes builds to fail citing [REDACTED: solved the problem, setuptools needed to be pinned in the CI, oops!]

configuration error: `project.license` must be valid exactly by one definition (2 matches found):

    - keys:
        'file': {type: string}
      required: ['file']
    - keys:
        'text': {type: string}
      required: ['text']

did you see this at all with mkl_fft? Only happens in CI as well...

@ndgrigorian ndgrigorian requested a review from vtavana April 13, 2025 19:54
@ndgrigorian ndgrigorian changed the title Migrate random to pyproject toml from setup.py Migrate random to pyproject.toml from setup.py Apr 13, 2025
@ndgrigorian ndgrigorian force-pushed the migrate-random-to-pyproject-toml branch from 5ab1a3c to 9abf583 Compare April 13, 2025 20:22
@ndgrigorian ndgrigorian force-pushed the migrate-random-to-pyproject-toml branch from 9abf583 to b9544d8 Compare April 13, 2025 20:28
@ndgrigorian ndgrigorian marked this pull request as ready for review April 13, 2025 20:32
@ndgrigorian
Copy link
Collaborator Author

@ekomarova
I've updated the build scripts here (similar to some previous changes in mkl_fft) to avoid directly calling setup.py since it's been deprecated for years anyway

Also fix formatting in pyproject.toml
@ekomarova
Copy link
Collaborator

Thanks, @ndgrigorian! As soon as there is a tag that includes these changes, I can rebuild mkl_random to include this in the internal recipe

Copy link
Collaborator

@vtavana vtavana left a comment

Choose a reason for hiding this comment

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

Thank you, @ndgrigorian!

@@ -52,7 +53,7 @@ jobs:
- name: Install mkl_random dependencies
shell: bash -l {0}
run: |
pip install cython setuptools pytest pytest-cov
pip install cython setuptools">=70.1" wheel
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is setuptools">=77" in other places. Was it intended to have a bit different dependency?

@@ -52,7 +53,7 @@ jobs:
- name: Install mkl_random dependencies
shell: bash -l {0}
run: |
pip install cython setuptools pytest pytest-cov
pip install cython setuptools">=70.1" wheel
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to update docs/source/maintenance/index.rst also with min setuptools version required?

architecture: x64
- name: Install sphinx dependencies
if: ${{ !github.event.pull_request || github.event.action != 'closed' }}
shell: bash -l {0}
run: |
pip install numpy cython setuptools scikit-build cmake sphinx sphinx_rtd_theme furo pydot graphviz sphinxcontrib-programoutput sphinxcontrib-googleanalytics sphinx_design
pip install numpy cython setuptools">=77" scikit-build cmake sphinx sphinx_rtd_theme furo pydot graphviz sphinxcontrib-programoutput sphinxcontrib-googleanalytics sphinx_design
- name: Checkout repo
uses: actions/[email protected]
with:
Copy link
Collaborator

Choose a reason for hiding this comment

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

on line 56, we have python setup.py develop should we replace with pip install like other places?

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.

4 participants