task: module CLI patching methods#319
Conversation
jharlow-intel
requested review from
antonwolfy,
ndgrigorian and
xaleryb
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a user-facing CLI for enabling/disabling mkl_fft’s NumPy FFT patch, aiming to support both persistent patching (python -m mkl_fft patch ...) and one-shot execution (python -m mkl_fft with_patch ...) alongside updated README guidance.
Changes:
- Introduces
python -m mkl_fft patch install|uninstall|statusfor persistent patch management via a.pthfile. - Introduces
python -m mkl_fft with_patch <command> ...intended to run a single command with temporary patching. - Adds README documentation and unit tests for the persistent patch helper functions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents persistent patching, one-shot execution, and programmatic patch APIs. |
| mkl_fft/main.py | Adds python -m mkl_fft ... dispatcher for patch and with_patch subcommands. |
| mkl_fft/patch.py | Implements persistent patch install/uninstall/status via writing/removing a .pth file. |
| mkl_fft/with_patch.py | Implements “one-shot” patch execution by spawning a subprocess with modified environment. |
| mkl_fft/tests/test_cli.py | Adds tests for install_patch, uninstall_patch, and check_status using a mocked .pth path. |
jharlow-intel
force-pushed
the
task/SAT-8469
branch
from
f73757a to
15c3ebb
Compare
|
@antonwolfy @ndgrigorian ready for review, I'll open this for the other 2 mkl packages once this gets merged. Tried to get thorough CLI testing added to the meta.yaml, but conda seems to do something weird with the PYTHONPATH while running the tests so I had to run a lot of integration tests by hand |
|
|
||
| """Command-line interface for mkl_fft.""" | ||
|
|
||
| import sys |
There was a problem hiding this comment.
I think we should use argparse here, dpctl is a good example
https://github.com/IntelPython/dpctl/blob/master/dpctl/__main__.py
it's a bit cleaner
There was a problem hiding this comment.
Agree, then it would be something like: python -m mkl_fft --patch [args]
| pth_path = get_pth_path() | ||
|
|
||
| if pth_path.exists(): | ||
| print(f"Persistent patch already installed at {pth_path}") |
There was a problem hiding this comment.
I guess it would be proper to have that as a warning, rather then default trace to the console.
There was a problem hiding this comment.
agreed, that does make more sense
| print(f"Persistent patch installed at {pth_path}") | ||
| print() | ||
| print("NumPy FFT will now use MKL-accelerated implementations in all") | ||
| print("Python sessions. To disable, run:") | ||
| print(" python -m mkl_fft patch uninstall") |
There was a problem hiding this comment.
I believe it should be only traced under verbose mode
| def test_install_patch_already_installed(mock_pth_path, capsys): | ||
| """Test installing patch when already installed.""" | ||
| install_patch() | ||
| install_patch() |
There was a problem hiding this comment.
It should be warning raised at second installation and then we will verify that here through appropriate pytest i/f.
|
|
||
| """Command-line interface for mkl_fft.""" | ||
|
|
||
| import sys |
There was a problem hiding this comment.
Agree, then it would be something like: python -m mkl_fft --patch [args]
| print() | ||
| print("Examples:") | ||
| print(" python -m mkl_fft patch install") | ||
| print(" python -m mkl_fft with_patch python script.py") |
There was a problem hiding this comment.
Why do we need the second python in the command? Is there any use case when we will run something different from python file?
There was a problem hiding this comment.
It might be helpful also to support something like:
python -m mkl_fft with_patch "import numpy as np; ...; res = np.fft.fft(data); ..."
| install_patch() | ||
|
|
||
| captured = capsys.readouterr() | ||
| assert "already installed" in captured.out |
There was a problem hiding this comment.
Do we have a better way to check the patch is enabled through importing mkl_fft and checking mkl_fft.is_patched() result?
| @@ -0,0 +1,33 @@ | |||
| # Copyright (c) 2017, Intel Corporation | |||
There was a problem hiding this comment.
Should we use the current year for new files?
| # Copyright (c) 2017, Intel Corporation | |
| # Copyright (c) 2026, Intel Corporation |
| print("Python sessions. To disable, run:") | ||
| print(" python -m mkl_fft patch uninstall") | ||
| except OSError as e: | ||
| print(f"Error installing patch: {e}") |
There was a problem hiding this comment.
Would it be better to handle through raising an exception?
adds
python -m mkl_fft patch install/uninstall/statusmethods, and alsopython -m mkl_fft with_patchConvenience CLI functions that will make it way easier to let users have automagic mkl_fft optimizations in their workflows, such as if they want to run npbench suites but with mkl_fft optimizations.
Tested locally pretty exhaustively, everything worked pretty well. A lot of documentation is claude code and it was way too verbose. I deleted and edited a lot of it, but please take a look at it all and make sure it's correct.
If this gets merged, I'll do the same for mkl_random and mkl_umath