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

Remove deprecation warnings from documentation #38580

Closed

Conversation

thecaligarmo
Copy link
Contributor

Fixes #37664

This will muffle the DeprecationWarning warnings that appear during (most) of the building of the documentation. In particular it removes all of the lazy_import warnings and some additional ones.

In addition, I've gone through and updated the traverse methods to findall for docutils in order to stop the deprecation warnings from those. (traverse returns a list of all nodes whereas findall returns an iterator and so it should also speed things up slightly)

📝 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.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

#37664

@thecaligarmo
Copy link
Contributor Author

I forgot to add #38468 so I've added it. Hopefully this time the docs should run ok.

Copy link

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

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 31, 2024

This is to suppress generated deprecation warnings.

Using warning filters seems simple and clean: https://docs.python.org/3/library/warnings.html

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 31, 2024

This does the trick.

--- a/src/sage_docbuild/sphinxbuild.py
+++ b/src/sage_docbuild/sphinxbuild.py
@@ -307,6 +307,11 @@ def runsphinx():
     saved_stdout = sys.stdout
     saved_stderr = sys.stderr
 
+    if not sys.warnoptions:
+        import warnings
+        original_filters = warnings.filters[:]
+        warnings.simplefilter("ignore", category=DeprecationWarning)
+
     try:
         sys.stdout = SageSphinxLogger(sys.stdout, os.path.basename(output_dir))
         sys.stderr = SageSphinxLogger(sys.stderr, os.path.basename(output_dir))
@@ -323,3 +328,6 @@ def runsphinx():
         sys.stderr = saved_stderr
         sys.stdout.flush()
         sys.stderr.flush()
+
+    if not sys.warnoptions:
+        warnings.filters = original_filters[:]

@thecaligarmo
Copy link
Contributor Author

This is to suppress generated deprecation warnings.

Using warning filters seems simple and clean: https://docs.python.org/3/library/warnings.html

Yup, that's what the branch does. It adds:

import warnings
with warnings.catch_warnings():
    warnings.filterwarnings("ignore", category=DeprecationWarning)

in two different locations to suppress the generated deprecation warning

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 31, 2024

To test

$ make doc-clean
...
$ sage --docbuild reference/cryptography inventory 
[cryptogra] building [inventory]: targets for 25 source files that are out of date
[cryptogra] updating environment: [new config] 25 added, 0 changed, 0 removed
[cryptogra] The inventory file is in local/share/doc/sage/inventory/en/reference/cryptography.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 31, 2024

This seems to be the difference from your code: My code applies the warning filter to sphinx itself. Your code applies the filter to the sage code internally imported by sphinx.

So in my case, a possible side effect is that any deprecation warning that has origin in sphinx will also be "ignored". Perhaps this is harmless...

@thecaligarmo
Copy link
Contributor Author

This seems to be the difference from your code: My code applies the warning filter to sphinx itself. Your code applies the filter to the sage code internally imported by sphinx.

Yes, you ended up commenting multiple times and I had only seen the first one. In addition you had mentioned to use something I had already used adding to my confusion hence why I mentioned what I had done.

The reason I didn't want to stop all deprecation warnings is because we do still want to catch some of them. For example the traverse deprecation of sphinx. Ignoring DeprecationWarnings are fine up until you're doing it on an external package and then that function gets fully deprecated and the code breaks down. Although normally deprecated functions stay for a very long time, I think it's safer to not get rid of every single deprecation warning so that we can see when a problem is starting to form and we can fix it before it becomes a massive problem.

If people would prefer us to remove every single one, no matter the source, I can go ahead and change the branch to be something more similar to yours so that everything is completely removed.

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 31, 2024

Sorry. I didn't look carefully at first because it seemed that there are many changes, and I immediately started to look around for a "simple" solution. Now I know that most of the changes are indentations :-)

Yes there is a risk in ignoring all deprecation warnings from sphinx. But practically, I think the risk is negligible.

If you don't want to take my code, then I will open my own PR. Then a reviewer will decide on either one.

I am okay anyway. I hope you are okay too!

@kwankyu
Copy link
Collaborator

kwankyu commented Aug 31, 2024

By the way

warnings.filterwarnings("ignore", category=DeprecationWarning, module='sphinx.util.inspect')  

also works. Hence python seems to think the warning is from sphinx. This specific filter will reduce the risk we were talking.

As it is most likely that Matthias will review any PR on this issue, I am okay with asking Matthias first.

@mkoeppe ?

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 4, 2024

I created #38608 with the suggested code.

Let's leave the choice to Matthias.

@thecaligarmo
Copy link
Contributor Author

@kwankyu Your suggested code is precisely what I was wanting to do, but I didn't do because you had said you wanted all DeprecationWarning warnings to be filtered instead of just some of them. Since you've now changed your mind and are ok with just the lazy_import ones being removed, I'll go ahead and close the ticket and we can use yours.

@thecaligarmo
Copy link
Contributor Author

Not sure how to delete a pull request, but this pull request needs closing as we will be implementing #38608 instead.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 4, 2024

@kwankyu Your suggested code is precisely what I was wanting to do, but I didn't do because you had said you wanted all DeprecationWarning warnings to be filtered instead of just some of them. Since you've now changed your mind and are ok with just the lazy_import ones being removed, I'll go ahead and close the ticket and we can use yours.

I am not sure about the history. Anyway, thanks for your understanding.

@kwankyu
Copy link
Collaborator

kwankyu commented Sep 4, 2024

Not sure how to delete a pull request, but this pull request needs closing as we will be implementing #38608 instead.

We can't delete a pull request. It is either merged or closed.

vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 5, 2024
… build

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37664.

from the log of the doc build workflow of this PR,
```
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/cryptography inventory
  [sagemath_doc_html-none] [spkg-install] [cryptogra] Converting
`source_suffix = '.rst'` to `source_suffix = {'.rst':
'restructuredtext'}`.
  [sagemath_doc_html-none] [spkg-install] [cryptogra] building
[inventory]: targets for 25 source files that are out of date
  [sagemath_doc_html-none] [spkg-install] [cryptogra] updating
environment: [new config] 25 added, 0 changed, 0 removed
  [sagemath_doc_html-none] [spkg-install] [cryptogra] The inventory file
is in ../../local/share/doc/sage/inventory/en/reference/cryptography.
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/curves inventory
```

This PR is based on sagemath#38580 and replaces it. Thank @thecaligarmo for
allowing this arrangement.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38608
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 6, 2024
… build

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37664.

from the log of the doc build workflow of this PR,
```
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/cryptography inventory
  [sagemath_doc_html-none] [spkg-install] [cryptogra] Converting
`source_suffix = '.rst'` to `source_suffix = {'.rst':
'restructuredtext'}`.
  [sagemath_doc_html-none] [spkg-install] [cryptogra] building
[inventory]: targets for 25 source files that are out of date
  [sagemath_doc_html-none] [spkg-install] [cryptogra] updating
environment: [new config] 25 added, 0 changed, 0 removed
  [sagemath_doc_html-none] [spkg-install] [cryptogra] The inventory file
is in ../../local/share/doc/sage/inventory/en/reference/cryptography.
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/curves inventory
```

This PR is based on sagemath#38580 and replaces it. Thank @thecaligarmo for
allowing this arrangement.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38608
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
… build

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37664.

from the log of the doc build workflow of this PR,
```
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/cryptography inventory
  [sagemath_doc_html-none] [spkg-install] [cryptogra] Converting
`source_suffix = '.rst'` to `source_suffix = {'.rst':
'restructuredtext'}`.
  [sagemath_doc_html-none] [spkg-install] [cryptogra] building
[inventory]: targets for 25 source files that are out of date
  [sagemath_doc_html-none] [spkg-install] [cryptogra] updating
environment: [new config] 25 added, 0 changed, 0 removed
  [sagemath_doc_html-none] [spkg-install] [cryptogra] The inventory file
is in ../../local/share/doc/sage/inventory/en/reference/cryptography.
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/curves inventory
```

This PR is based on sagemath#38580 and replaces it. Thank @thecaligarmo for
allowing this arrangement.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38608
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 8, 2024
… build

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37664.

from the log of the doc build workflow of this PR,
```
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/cryptography inventory
  [sagemath_doc_html-none] [spkg-install] [cryptogra] Converting
`source_suffix = '.rst'` to `source_suffix = {'.rst':
'restructuredtext'}`.
  [sagemath_doc_html-none] [spkg-install] [cryptogra] building
[inventory]: targets for 25 source files that are out of date
  [sagemath_doc_html-none] [spkg-install] [cryptogra] updating
environment: [new config] 25 added, 0 changed, 0 removed
  [sagemath_doc_html-none] [spkg-install] [cryptogra] The inventory file
is in ../../local/share/doc/sage/inventory/en/reference/cryptography.
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/curves inventory
```

This PR is based on sagemath#38580 and replaces it. Thank @thecaligarmo for
allowing this arrangement.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38608
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 10, 2024
… build

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37664.

from the log of the doc build workflow of this PR,
```
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/cryptography inventory
  [sagemath_doc_html-none] [spkg-install] [cryptogra] Converting
`source_suffix = '.rst'` to `source_suffix = {'.rst':
'restructuredtext'}`.
  [sagemath_doc_html-none] [spkg-install] [cryptogra] building
[inventory]: targets for 25 source files that are out of date
  [sagemath_doc_html-none] [spkg-install] [cryptogra] updating
environment: [new config] 25 added, 0 changed, 0 removed
  [sagemath_doc_html-none] [spkg-install] [cryptogra] The inventory file
is in ../../local/share/doc/sage/inventory/en/reference/cryptography.
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/curves inventory
```

This PR is based on sagemath#38580 and replaces it. Thank @thecaligarmo for
allowing this arrangement.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38608
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 12, 2024
… build

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37664.

from the log of the doc build workflow of this PR,
```
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/cryptography inventory
  [sagemath_doc_html-none] [spkg-install] [cryptogra] Converting
`source_suffix = '.rst'` to `source_suffix = {'.rst':
'restructuredtext'}`.
  [sagemath_doc_html-none] [spkg-install] [cryptogra] building
[inventory]: targets for 25 source files that are out of date
  [sagemath_doc_html-none] [spkg-install] [cryptogra] updating
environment: [new config] 25 added, 0 changed, 0 removed
  [sagemath_doc_html-none] [spkg-install] [cryptogra] The inventory file
is in ../../local/share/doc/sage/inventory/en/reference/cryptography.
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/curves inventory
```

This PR is based on sagemath#38580 and replaces it. Thank @thecaligarmo for
allowing this arrangement.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38608
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
vbraun pushed a commit to vbraun/sage that referenced this pull request Sep 14, 2024
… build

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Fixes sagemath#37664.

from the log of the doc build workflow of this PR,
```
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/cryptography inventory
  [sagemath_doc_html-none] [spkg-install] [cryptogra] Converting
`source_suffix = '.rst'` to `source_suffix = {'.rst':
'restructuredtext'}`.
  [sagemath_doc_html-none] [spkg-install] [cryptogra] building
[inventory]: targets for 25 source files that are out of date
  [sagemath_doc_html-none] [spkg-install] [cryptogra] updating
environment: [new config] 25 added, 0 changed, 0 removed
  [sagemath_doc_html-none] [spkg-install] [cryptogra] The inventory file
is in ../../local/share/doc/sage/inventory/en/reference/cryptography.
  [sagemath_doc_html-none] [spkg-install] sage --docbuild --no-pdf-links
reference/curves inventory
```

This PR is based on sagemath#38580 and replaces it. Thank @thecaligarmo for
allowing this arrangement.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#38608
Reported by: Kwankyu Lee
Reviewer(s): Matthias Köppe
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.

Muffle DeprecationWarning during docbuild
3 participants