-
Notifications
You must be signed in to change notification settings - Fork 54
CLI options for controlling Cython extension compliation #567
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
base: develop
Are you sure you want to change the base?
Conversation
PYGSTI_CYTHON_SKIP being defined will fully skip extension compilation. PYGSTI_CYTHON_EXCLUDE_FAILURES will run cythonize with exclude_failures=True. Also removes the unneeded setup.cfg, and applies PYGSTI_CYTHON_SKIP to the "No Cython" GitHub Actions.
@@ -69,7 +69,7 @@ jobs: | |||
- name: Install package (No Cython) | |||
if: ${{ inputs.use-cython != 'true' }} | |||
run: | | |||
python -m pip install -e .[testing_no_cython] | |||
PYGSTI_CYTHON_SKIP=1 python -m pip install -e .[testing_no_cython] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should fix #559
@@ -1208,6 +1207,8 @@ def _draw_graph(G, node_label_key='label', edge_label_key='promotion_cost', figu | |||
An optional size specifier passed into the matplotlib figure | |||
constructor to set the plot size. | |||
""" | |||
import matplotlib.pyplot as plt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved import that broke minimal install. Matplotlib is an optional dependency currently.
"Programming Language :: Python", | ||
"Topic :: Scientific/Engineering :: Physics", | ||
"Operating System :: Microsoft :: Windows", | ||
"Operating System :: MacOS :: MacOS X", | ||
"Operating System :: Unix" | ||
] | ||
license = "Apache-2.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous classifier was deprecated in favor of this.
setup.cfg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was unneeded - the [metadata] section is redundant with pyproject.toml, [bdist_wheel] for universal is actually deprecated with Python 2.7 EOL and would cause us issues in Aug 2025, and [nosetests] is out-of-date since we migrated to pytest.
@@ -55,12 +54,219 @@ def build_extensions(self): | |||
ext.extra_compile_args = args | |||
build_ext.build_extensions(self) | |||
|
|||
# Check if environment can try to build extensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of this is just moving stuff around, I'll note the critical changes.
extra_link_args=["-std=c++11"] | ||
) | ||
] | ||
extensions = cythonize(ext_modules, compiler_directives={'language_level': "3"}, exclude_failures="PYGSTI_CYTHON_EXCLUDE_FAILURES" in os.environ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we went from exclude_failures=True to a check against PYGSTI_CYTHON_EXCLUDE_FAILURES
except ImportError: | ||
warn("Extensions build tools are not available. Installing without Cython extensions...") | ||
extensions = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still don't build extensions if no Cython. However, as mentioned in my main message, this should basically never trigger because it has to do with the build-system requirements, not the user-specified optional dependencies.
# Extension compilation failed | ||
warn("Error in extension compilation. Installing without Cython extensions...") | ||
setup_with_extensions() | ||
except SystemExit as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have this try-except block so that we can tell the user about the env variables. However, we now re-raise the exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work figuring this all out. I don't have any questions or concerns about any of these changes. Thanks, @sserita!
This should fix both #566 and #559, giving the user better control over when Cython extensions are installed (and therefore us better control in our GitHub runners).
First, a clarification that took me a while to understand but is critical to understanding the issue in #559: whether or not pyGSTi's Cython extensions get compiled have nothing to do with which optional dependencies get installed, and everything to do with what is present in pip's isolated build system at install time. This, in turn, uses the
build-system.requires
section ofpyproject.toml
. SinceCython
andnumpy
are included here, this means that pyGSTi will attempt to build Cython extensions even if the user is not specifyingpygsti[extensions]
(or a superset, like[testing]
or[complete]
).It is not possible to make the
build-system.requires
field dynamic, so we instead always install Cython and provide a way for the user to turn this off via environment variables.Solution
During installation, pyGSTi will now check for two environment variables:
PYGSTI_CYTHON_SKIP
andPYGSTI_CYTHON_EXCLUDE_FAILURES
. If the first is defined, then extension compilation will be skipped entirely. If the second is defined, then extension compilation will be performed withexclude_failures=True
defined - this used to be the default but can suppress errors in the case where one of the extensions fails but the others succeed. (This is uncommon and is most likely to be useful to developers who are writing Cython extensions, but I opted for the more verbose failure behavior by default).Aside: Attempted PEP-517 Solution
In theory, it should be possible to do a pip-flag-based option via
--config-settings
. See an example package listed here using this for optional Cythonization.However, I could not figure out how to get this working and gave up after several hours of banging my head against it. The CLI flag seems to be working fine.
New Behavior Examples
There is still a try-except block around the compilation. However, this now re-raises the error and I hope will prevent suppression of any error messages. We do still need it so that we can provide the user with the error message telling them about the new flags.
I'll use
CC=broken
to emulate a missing compiler that would trigger Cython compilation errors for testing. We can see that now we have our error message at the bottom.Note that there is the full stack trace, an error message telling the user what environment variables should be used to avoid this issue, and then the actual relevant line of the error message.
Now with our newly introduced flag:
No error! And we can check none of the stuff got compiled, and that it does if we don't include the variable.