Skip to content
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

build: show all cython warnings #37885

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tornaria
Copy link
Contributor

Motivated by #37792 (comment), change cython configuration so we show all warnings.

This will lead to a number of warnings, but it's IMO better to know about that and works towards fixing as much warnings as possible (and we have a fair number of warnings that are indeed relevant and not just false positives).

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 27, 2024

This change won't take effect for most people because the editable build (via src/setup.py) does not go through sage_build_cython

Copy link

github-actions bot commented Apr 28, 2024

Documentation preview for this PR (built with commit 4dbb878; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@tornaria
Copy link
Contributor Author

What is going on here:

  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************
  [sagemath_categories-10.4.beta4] [spkg-check] File "sage/arith/power.pyx", line 49, in sage.arith.power.generic_power
  [sagemath_categories-10.4.beta4] [spkg-check] Failed example:
  [sagemath_categories-10.4.beta4] [spkg-check]     F = Zmod(5)
  [sagemath_categories-10.4.beta4] [spkg-check] Exception raised:
  [sagemath_categories-10.4.beta4] [spkg-check]     Traceback (most recent call last):
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 714, in _run
  [sagemath_categories-10.4.beta4] [spkg-check]         self.compile_and_execute(example, compiler, test.globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
  [sagemath_categories-10.4.beta4] [spkg-check]         exec(compiled, globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "<doctest sage.arith.power.generic_power[6]>", line 1, in <module>
  [sagemath_categories-10.4.beta4] [spkg-check]         F = Zmod(Integer(5))
  [sagemath_categories-10.4.beta4] [spkg-check]     NameError: name 'Zmod' is not defined
  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************
  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************
  [sagemath_categories-10.4.beta4] [spkg-check] File "sage/categories/additive_monoids.py", line 41, in sage.categories.additive_monoids.AdditiveMonoids
  [sagemath_categories-10.4.beta4] [spkg-check] Failed example:
  [sagemath_categories-10.4.beta4] [spkg-check]     C.Algebras(QQ).is_subcategory(AlgebrasWithBasis(QQ))
  [sagemath_categories-10.4.beta4] [spkg-check] Exception raised:
  [sagemath_categories-10.4.beta4] [spkg-check]     Traceback (most recent call last):
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 714, in _run
  [sagemath_categories-10.4.beta4] [spkg-check]         self.compile_and_execute(example, compiler, test.globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "/sage/pkgs/sagemath-categories/.tox/sagepython-sagewheels-nopypi-norequirements/lib/python3.10/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
  [sagemath_categories-10.4.beta4] [spkg-check]         exec(compiled, globs)
  [sagemath_categories-10.4.beta4] [spkg-check]       File "<doctest sage.categories.additive_monoids.AdditiveMonoids[6]>", line 1, in <module>
  [sagemath_categories-10.4.beta4] [spkg-check]         C.Algebras(QQ).is_subcategory(AlgebrasWithBasis(QQ))
  [sagemath_categories-10.4.beta4] [spkg-check]     NameError: name 'QQ' is not defined
  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************

etc...

@tornaria
Copy link
Contributor Author

This change won't take effect for most people because the editable build (via src/setup.py) does not go through sage_build_cython

Why can't src/setup.py use the same code to build cython extensions? In any case, the same line can be added to src/setup.py.

The review question here is: what do you think about enabling all cython warnings as I suggest here? This adds a lot of cython warnings, but at least the Unraisable exception... warning seems a real problem (see #37792 (comment) -- after all of those are fixed maybe this warning should be turned into an error).

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 28, 2024

What is going on here:

  [sagemath_categories-10.4.beta4] [spkg-check] **********************************************************************

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 28, 2024

Why can't src/setup.py use the same code to build cython extensions?

Got to ask the author about that.

@tornaria
Copy link
Contributor Author

The review question here is: what do you think about enabling all cython warnings as I suggest here? This adds a lot of cython warnings, but at least the Unraisable exception... warning seems a real problem (see #37792 (comment) -- after all of those are fixed maybe this warning should be turned into an error).

@dimpase
Copy link
Member

dimpase commented Apr 29, 2024

"unraiseable" should definitely be turned on.

warnings which are certainly bogus - better not. But OK, all on is good too.

@tornaria tornaria force-pushed the cython-show-all-warnings branch from cebc320 to 4dbb878 Compare April 30, 2024 00:35
@tornaria
Copy link
Contributor Author

I updated the PR so that all cython warnings are enabled, both in sage_setup and in src/setup.py (for editable builds).

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 30, 2024

Thanks! I'll try it out and take a look.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 6, 2024

The output from all the warnings seems somewhat excessive. Perhaps here we should at least try to fix the warnings that come in from the most commonly included .pxd files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants