Skip to content

MAINT: Use python shim and clean up CI script calls #29324

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 1 commit into
base: main
Choose a base branch
from

Conversation

gaurang98671
Copy link

MAINT: Use python shim and clean up CI script calls

  1. Replaced hardcoded python3.11 and python3 with python
  2. Redirected doc build failure message to stderr.
  3. Added executable premissions for tools/refguide_check.py

@gaurang98671
Copy link
Author

gaurang98671 commented Jul 5, 2025

Was skimming through this repo and stumbled upon CI/CD .yml. Added some minor refactors which I think would make it a bit cleaner :)

Copy link
Member

@melissawm melissawm left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, although I can't confirm the python command as I'm not sure how python is called in the circleci image..

@melissawm melissawm moved this to Awaiting a code review in NumPy first-time contributor PRs Jul 7, 2025
@gaurang98671
Copy link
Author

gaurang98671 commented Jul 7, 2025

@melissawm I confirmed it, all the python commands in that image will point to a corresponding bash scripts with same name. These scripts are responsible for invoking the actual Python binary.

image

All the python shims(python, python3 and python3.11) points to same binary

image

And python and python3 are symlinked to python3.11 binary
image

@@ -52,7 +52,7 @@ jobs:
- run:
name: build NumPy
command: |
python3.11 -m venv venv
python -m venv venv
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to leave this as is. We secifically must build the dicumentation with python 3.11 since we have distutils extensions. I wouldn’t want the version to change out from under us without it causing an obvious error

@@ -69,7 +69,7 @@ jobs:
# Don't use -q, show warning summary"
SPHINXOPTS="-W -n" spin docs
if [[ $(find doc/build/html -type f | wc -l) -lt 1000 ]]; then
echo "doc build failed: doc/build/html is empty"
echo "doc build failed: doc/build/html is empty" >&2
Copy link
Member

Choose a reason for hiding this comment

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

What is wrong with the way this is? Why force stderr?

Copy link
Author

Choose a reason for hiding this comment

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

It might be helpful if the logs are being streamed to any observability tool where it is possible to monitor it easily based on stdout or stderr stream. Happy to remove it if it seems unnecessary.

echo calling python3 tools/refguide_check.py -v
python3 tools/refguide_check.py -v
echo "calling ./tools/refguide_check.py -v"
./tools/refguide_check.py -v
Copy link
Member

Choose a reason for hiding this comment

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

If anything, I would prefer this specifically uses python 3.11 like above

Copy link
Author

Choose a reason for hiding this comment

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

They all are symlinked to same python binary anyway configured in cimg/python:3.11.10 image. Thought it migth be cleaner to stick to just one common invocation format. Like how we are invoking pip and not pip3 or pip3.11. Unless I'm missing some obvious side effects.

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

FWIW I agree with @mattip - the intentional explicitness of Python version is useful in this case. If anything were to change, I'd vote to add a comment to explain why things are the way they are!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting a code review
Development

Successfully merging this pull request may close these issues.

4 participants