Follow up #36741: improve code structure#37737
Conversation
Thanks, too! |
|
I'm getting this in parallel tests, but passes in isolation: |
|
Documentation preview for this PR (built with commit 0f87bb8; changes) is ready! 🎉 |
I can't reproduce this. Could you give us more details and/or share the head of the tree where you see this? |
|
I got this twice in a row when running make distclean && make && make ptestlong But in general can we make the test maybe less fragile, is it really important that those 4 items have to be consecutively in there? Maybe you just want to check that one is in there so hidden works at all? |
The fact that |
|
The current commit addresses #37905 in the context of this PR. As mentioned in #37857 (comment) the connection betwee the content of the variable Note that there is no real need to cache the result of Likewise, there is no need to set |
|
test-mod shows this failure. This is a separate test of the modularized distribution sagemath-categories. |
sagemathgh-37737: Follow up sagemath#36741: improve code structure <!-- ^ 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". --> From sagemath#36741 (comment) > Overall, I would suggest to remove the direct and unconditional use of multiprocessing from sage.features. > Perhaps sage.doctest can put such Value attributes into features that are to be hidden. This PR implements the suggestion made in sagemath#36741 (comment) > I looked a little bit into it and here's an idea: > > * In features, implement a simple hide() / unhide() / is_hidden() interface. > * The only state is _hidden, not shared (for parallel doctesting this will be set before fork so it's ok) > * Implement AvailableSoftware.hidden() similar to AvailableSoftware.seen() so the logic stays in that class and internals don't leak to sage.doctest.control (if necessary use _seen[idx] to record hidden state and/or number of hidings, or add another shared array) Fixes sagemath#37905 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] 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 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#37737 Reported by: Sebastian Oehms Reviewer(s): Matthias Köppe, Sebastian Oehms
sagemathgh-37737: Follow up sagemath#36741: improve code structure <!-- ^ 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". --> From sagemath#36741 (comment) > Overall, I would suggest to remove the direct and unconditional use of multiprocessing from sage.features. > Perhaps sage.doctest can put such Value attributes into features that are to be hidden. This PR implements the suggestion made in sagemath#36741 (comment) > I looked a little bit into it and here's an idea: > > * In features, implement a simple hide() / unhide() / is_hidden() interface. > * The only state is _hidden, not shared (for parallel doctesting this will be set before fork so it's ok) > * Implement AvailableSoftware.hidden() similar to AvailableSoftware.seen() so the logic stays in that class and internals don't leak to sage.doctest.control (if necessary use _seen[idx] to record hidden state and/or number of hidings, or add another shared array) Fixes sagemath#37905 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] 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 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#37737 Reported by: Sebastian Oehms Reviewer(s): Matthias Köppe, Sebastian Oehms
|
Thanks! |
sagemathgh-37857: Replace special handling of optional extensions (bliss, coxeter3, ....) <!-- ^ 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". --> We relieve the distribution **sagemath-standard** (in both versions - `SAGE_ROOT/pkgs/sagemath-standard` and `SAGE_ROOT/src`) from the duty to build "optional extensions" based on what Sage packages are installed. The installation is now done uniformly using the modularized distributions **sagemath-bliss**, **sagemath-coxeter3** etc. We introduce the missing features `coxeter3` and `sirocco` so that the doctester does not have to rely on the sage-the-distro installation records any more. The wheels of the distributions now build correctly even when not going through building an sdist first, which previously was required to apply MANIFEST-based filtering. This is achieved using the new `sage_setup.command.build_py`. User-visible change: - To install these options, use `./configure --enable-sagemath_bliss` before building, or use `./sage -i sagemath_bliss` or `make sagemath_bliss`. ### 📝 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 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: ... --> - Depends on sagemath#37737 (merged here) - Depends on sagemath#37973 (merged here) URL: sagemath#37857 Reported by: Matthias Köppe Reviewer(s): François Bissey
sagemathgh-37857: Replace special handling of optional extensions (bliss, coxeter3, ....) <!-- ^ 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". --> We relieve the distribution **sagemath-standard** (in both versions - `SAGE_ROOT/pkgs/sagemath-standard` and `SAGE_ROOT/src`) from the duty to build "optional extensions" based on what Sage packages are installed. The installation is now done uniformly using the modularized distributions **sagemath-bliss**, **sagemath-coxeter3** etc. We introduce the missing features `coxeter3` and `sirocco` so that the doctester does not have to rely on the sage-the-distro installation records any more. The wheels of the distributions now build correctly even when not going through building an sdist first, which previously was required to apply MANIFEST-based filtering. This is achieved using the new `sage_setup.command.build_py`. User-visible change: - To install these options, use `./configure --enable-sagemath_bliss` before building, or use `./sage -i sagemath_bliss` or `make sagemath_bliss`. ### 📝 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 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: ... --> - Depends on sagemath#37737 (merged here) - Depends on sagemath#37973 (merged here) URL: sagemath#37857 Reported by: Matthias Köppe Reviewer(s): François Bissey
From #36741 (comment)
This PR implements the suggestion made in #36741 (comment)
Fixes #37905
📝 Checklist
⌛ Dependencies