-
-
Notifications
You must be signed in to change notification settings - Fork 673
Add script to record known cyclic imports #40891
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
If desired, I can add a CI step to make sure that PRs don't introduce additional cycles (it's already prepared in the script with the "--check" option). |
Documentation preview for this PR (built with commit 815a5e4; changes) is ready! 🎉 |
This is great, thanks. Some of the cycles look a little over-simplified to me, though, e.g. [[cycle]]
modules = [
"sage.rings.rational",
"sage.rings.rational",
]
# Example exception:
# Traceback (most recent call last):
# File "/home/tobias/sage/tools/find_cyclic_imports.py", line 197, in import_module
# spec.loader.exec_module(module)
# File "<frozen importlib._bootstrap_external>", line 999, in exec_module
# File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
# File "/home/tobias/sage/src/sage/combinat/rigged_configurations/bij_type_A2_dual.py", line 43, in <module>
# from sage.rings.rational_field import QQ
# File "/home/tobias/sage/src/sage/rings/rational_field.py", line 56, in <module>
# from sage.rings.integer import Integer
# File "integer.pyx", line 1, in init sage.rings.integer
# File "rational.pyx", line 68, in init sage.rings.rational
# ImportError: cannot import name QQ Wouldn't this be rational_field -> integer -> rational -> rational_field? (There is a shorter rational <-> rational_field loop, just like with integer and integer_ring, but I we're probably only able to detect the first one that makes it crash.) Here is the other one that looks suspiciously short. It may actually be fixed by #40879 though. [[cycle]]
modules = [
"sage.rings.polynomial.polynomial_element",
"sage.rings.polynomial.polynomial_element",
]
# Example exception:
# Traceback (most recent call last):
# File "/home/tobias/sage/tools/find_cyclic_imports.py", line 199, in import_module
# spec.loader.exec_module(module)
# File "<frozen importlib._bootstrap_external>", line 999, in exec_module
# File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
# File "/home/tobias/sage/src/sage/modular/quatalg/brandt.py", line 213, in <module>
# from sage.modular.dirichlet import TrivialCharacter
# File "/home/tobias/sage/src/sage/modular/dirichlet.py", line 79, in <module>
# from sage.rings.power_series_ring import PowerSeriesRing
# File "/home/tobias/sage/src/sage/rings/power_series_ring.py", line 141, in <module>
# from sage.rings import (
# File "power_series_mpoly.pyx", line 6, in init sage.rings.power_series_mpoly
# File "multi_polynomial_ring_base.pyx", line 25, in init sage.rings.polynomial.multi_polynomial_ring_base
# File "/home/tobias/sage/src/sage/rings/polynomial/polynomial_ring.py", line 175, in <module>
# from sage.rings.polynomial.polynomial_singular_interface import PolynomialRing_singular_repr
# File "/home/tobias/sage/src/sage/rings/polynomial/polynomial_singular_interface.py", line 48, in <module>
# import sage.rings.finite_rings.finite_field_constructor
# File "/home/tobias/sage/src/sage/rings/finite_rings/finite_field_constructor.py", line 178, in <module>
# from sage.rings.polynomial.polynomial_element import Polynomial
# File "polynomial_element.pyx", line 122, in init sage.rings.polynomial.polynomial_element
# ImportError: cannot import name PolynomialRing_generic |
The problem here is that Cython imports only throw "# ImportError: cannot import name PolynomialRing_generic" without telling you where they tried to import that guy from. I don't know if there is a good way to find the module that defines this function/variable. Currently, the code traverses the imported modules and tries to find the symbol in one of them; but this usually doesn't work since that module is exactly in a half-loaded state. One could probably start a new subprocess, then load sage.all there first to resolve all circular imports and then search. Not sure if that's worth the work... but maybe you have a simpler idea? |
Introduce a new TOML file,
known-cyclic-imports.toml
, which contains a list of identified cyclic import patterns within Sage. The file is auto-generated by thetools/find_cyclic_imports.py
script and serves as a reference to help avoid circular import issues. Each entry includes the involved modules and an example traceback of the import error encountered.The script has a few limitations (eg it doesn't try to import Cython modules) and sometimes is unable to correctly extract the last import correctly. But it's a good start I would say.
No big surprises in the results: almost all cycles are related to categories, and a few other ones in rings.
Refs: #26254 (comment) (as a discussion of one of the cyclic imports) and is part of #33580
📝 Checklist
⌛ Dependencies