Skip to content

Conversation

@tornaria
Copy link
Contributor

Some failures I found when building and running as in #37138 (comment)

The first one is a missing # needs tdlib which is not noticed when building a wheel from the github source instead of via sdist, since tdlib.pyx is included in the former but not in the latter.

The second one triggers when one installs the wheel in a random directory (not site-packages, not a venv) and runs via PYTHONPATH. Then module names like sage.doctest.tests.tolerance are instead just tolerance so there's a mismatch. Very easy to fix writing ...tolerance instead.

📝 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.

This triggers when one installs `sagemath-standard` from the sdist, which
is missing `tdlib.pyx`; the issue is hidden when installing from the
source tree since then `tdlib.pyx` is installed with `sagemath-standard`,
in spite of it being in `sagemath-tdlib`.
This triggers when one is running sagemath not from site-packages (via
PYTHONPATH). In this case, the lines like
```
File "tolerance.rst", line ..., in sage.doctest.tests.tolerance
```
are instead printed as
```
File "tolerance.rst", line 6, in tolerance`
```
@tornaria tornaria requested a review from mkoeppe March 22, 2024 00:07
@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 22, 2024

Then module names like sage.doctest.tests.tolerance are instead just tolerance so there's a mismatch. Very easy to fix writing ...tolerance instead.

I think we can find a better fix for this, let me take a look. This may be the same issue as with all the PEP-420 issues -- finding the root is not well-defined. (But it's well-defined in the Sage layout because of our all*.py files.)

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 22, 2024

Indeed this is happening in sage.doctest.sources.get_basename, which is one of the last holdouts of our use of SAGE_SRC and SAGE_LIB.

@github-actions
Copy link

Documentation preview for this PR (built with commit 1f7f42b; changes) is ready! 🎉

@tornaria
Copy link
Contributor Author

age.doctest.sources.get_basename,

No, it's not fixed by that.

Is my fix here unreasonable?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 24, 2024

Is my fix here unreasonable?

If the basename was used only for printing these messages, I would probably not have bothered to look for a proper fix.
But it's also used as the identifier for the "stats", and thus for the "baseline_stats" mechanism.

@tornaria
Copy link
Contributor Author

Is my fix here unreasonable?

If the basename was used only for printing these messages, I would probably not have bothered to look for a proper fix. But it's also used as the identifier for the "stats", and thus for the "baseline_stats" mechanism.

But this is a "second level" test. We doctest sage.doctest.test whose identifier is ok, some doctest in that file changes directory to .../sage/doctest/tests/ and runs sage -t tolerance.rst in there (it passes the filename without any directory) and it's this run of sage -t tolerance.rst which fails because the identifier is printed incorrectly.

I don't think this "second level" is relevant for the "stats", or is it?

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 25, 2024

some doctest in that file changes directory to .../sage/doctest/tests/ and runs sage -t tolerance.rst in there (it passes the filename without any directory) and it's this run of sage -t tolerance.rst which fails because the identifier is printed incorrectly.

You are describing it correctly; except it's not just "printed" incorrectly. There's no such code that abbreviates identifiers further somehow, as far as I know.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 25, 2024

I agree that in this situation (running sage -t tolerance.rst), just the plain filename would conceivably also be the correct identifier -- but it really should not depend on the SAGE_LIB business. That's what #37647 attempts to fix.

I also agree that none of this is very important, and that nothing would be wrong with fixing it the way that you do here. It's just that I cannot resist opportunities for synergetic fixes.

@tornaria
Copy link
Contributor Author

I agree that in this situation (running sage -t tolerance.rst), just the plain filename would conceivably also be the correct identifier -- but it really should not depend on the SAGE_LIB business. That's what #37647 attempts to fix.

I also agree that none of this is very important, and that nothing would be wrong with fixing it the way that you do here. It's just that I cannot resist opportunities for synergetic fixes.

Makes sense, but since I wasn't able to reproduce it using get_basename(..) we are kind of blind on that. Sorry about that.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 25, 2024

No worries, I'll test it later today or tomorrow.

@tornaria
Copy link
Contributor Author

Ping @mkoeppe

I'm switching our package to build from pypi sdist, and I was hit by this:

sage -t --warn-long 4.0 --random-seed=306581033401793295942652738315483415678 /usr/lib/python3.12/site-packages/sage/misc/package_dir.py
**********************************************************************
File "/usr/lib/python3.12/site-packages/sage/misc/package_dir.py", line 116, in sage.misc.package_dir.read_distribution
Failed example:
    read_distribution(os.path.join(SAGE_SRC, 'sage', 'graphs', 'graph_decompositions', 'tdlib.pyx'))
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/lib/python3.12/site-packages/sage/doctest/forker.py", line 1146, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.package_dir.read_distribution[2]>", line 1, in <module>
        read_distribution(os.path.join(SAGE_SRC, 'sage', 'graphs', 'graph_decompositions', 'tdlib.pyx'))
      File "/usr/lib/python3.12/site-packages/sage/misc/package_dir.py", line 121, in read_distribution
        with open(src_file, encoding='utf-8', errors='ignore') as fh:
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    FileNotFoundError: [Errno 2] No such file or directory: '/usr/lib/python3.12/site-packages/sage/graphs/graph_decompositions/tdlib.pyx'
**********************************************************************
1 item had failures:
   1 of   5 in sage.misc.package_dir.read_distribution
    [35 tests, 1 failure, 0.02 s]

@tornaria tornaria requested a review from kiwifb April 29, 2024 00:19
@kiwifb
Copy link
Member

kiwifb commented Apr 29, 2024

I had not noticed that the doctest error about tdblib.pyx disappeared when I started to apply the line graft sage to fix the great sdist debacle for 10.4.beta3. It does cause this file to be shipped which is an issue in and off itself but as feedback to another issue.

@tornaria
Copy link
Contributor Author

For the record, I switched void-linux package to pypi sdist and it's working fine with this PR as seen in void-linux/void-packages#49571

@kiwifb
Copy link
Member

kiwifb commented Apr 29, 2024

The # needs tdlib stuff does work and I am happy with that bit. I do not see the other test failures in sage/doctest/test.py which means that for me that part makes no difference.

vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
sagemathgh-37650: src/sage/features/sagemath.py: Add feature SAGE_SRC
    
<!-- ^ 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". -->

This feature is for conditionalizing some doctests.

Fixes doctest failure reported in
sagemath#37645.

### 📝 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.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ 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#37650
Reported by: Matthias Köppe
Reviewer(s): François Bissey, Gonzalo Tornaría, Matthias Köppe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants