-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
Doctest hide option: Better detection of hidden packages #36741
Conversation
I agree |
See #37151. |
Would something like this work? def all_features():
- return [GapPackage("atlasrep", spkg="gap_packages"),
+ return [GapPackage("atlasrep", spkg="gap", type='standard'),
GapPackage("design", spkg="gap_packages"),
GapPackage("grape", spkg="gap_packages"),
GapPackage("guava", spkg="gap_packages"),
GapPackage("hap", spkg="gap_packages"),
- GapPackage("polycyclic", spkg="gap_packages"),
+ GapPackage("polycyclic", spkg="gap", type='standard'),
GapPackage("qpa", spkg="gap_packages"),
GapPackage("quagroup", spkg="gap_packages")] |
src/sage/features/__init__.py
Outdated
@@ -802,7 +808,7 @@ class StaticFile(FileFeature): | |||
To install no_such_file...you can try to run...sage -i some_spkg... | |||
Further installation instructions might be available at http://rand.om. | |||
""" | |||
def __init__(self, name, filename, *, search_path=None, **kwds): | |||
def __init__(self, name, filename, search_path=None, type='optional', **kwds): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the *
there was there intentionally. It marks the following parameters as keyword-only (cannot be passed as positional)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I will do this later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This id done, now! I took the occasion to also improve some doctest tags concerning the Gap package Grape which I came across.
Co-authored-by: Matthias Köppe <mkoeppe@math.ucdavis.edu>
Co-authored-by: Matthias Köppe <mkoeppe@math.ucdavis.edu>
Co-authored-by: Matthias Köppe <mkoeppe@math.ucdavis.edu>
Documentation preview for this PR (built with commit 03670bd; changes) is ready! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Thanks, as well! |
@soehms: this
breaks on musl libc. It seems there is a limit on how many Running it locally gives the exact same failure. And I can also reproduce with:
It seems BTW, the documentation of multiprocessing explicitly warns that |
Actually this:
works ok (same with As a matter of fact, is a lock really necessary? It seems to me that nothing too bad will happen if two threads try to increment at the same time since the value is only used for reporting and not to take any decision (also it seems this is not protected atm so not using a lock should be equivalent to what is done now). In summary, perhaps # For multiprocessing of doctests, the data self._num_hidings should be
# shared among subprocesses. Thus we use the Value class from the
# multiprocessing module (cf. self._seen of class AvailableSoftware)
from multiprocessing import Value
- self._num_hidings = Value('i', 0)
+ self._num_hidings = Value('i', 0, lock=False) is the simplest solution, with I don't understand why the lock used by default is worse than
|
Also on macOS I see new multiprocessing-related issues that may be coming from this PR |
Can confirm that this fixes the problem on macOS (repro: just Overall, I would suggest to remove the direct and unconditional use of |
Sorry for causing trouble again, especially that I missed the warning in the documentation!
I agree that this is not necessary. In addition, there even is no need to increase the value, since I finally refrained from printing the exact number of hidings. Unfortunately, I cannot work on this before Tueseday. If you don't want to wait, feel free to start on it. |
So, shall we get rid of the I looked a little bit into it and here's an idea:
I could do something like I described above, if you think it's ok, but I'd still wait for you to review it. Meanwhile, the |
Sounds good! Any solution you find that preserves the current behavior and improves the existing implementation is good for me. Many thanks! |
This affects musl libc and macos, see: sagemath#36741 (comment) We are working on a better fix to avoid using multiprocessing in sage.features, but this will do meanwhile. There is agreement that disabling the lock is completely harmless, since the counter is not really used for anything; disabling the lock fixes the issues that we had on 10.4.beta0.
This affects musl libc and macos, see: sagemath#36741 (comment) We are working on a better fix to avoid using multiprocessing in sage.features (cf sagemath#36741 (comment)), but this will do meanwhile. There is agreement that disabling the lock is completely harmless, since the counter is not really used for anything; disabling the lock fixes the issues that we had on 10.4.beta0. ### 📝 Checklist - [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. URL: sagemath#37702 Reported by: Gonzalo Tornaría Reviewer(s): Sebastian Oehms
This breaks tests in
|
See |
I suspect that in your test you had differences in |
I am using an unmodified |
<!-- ^ 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
<!-- ^ 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
<!-- ^ 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
This PR implements the suggestion made in #36696 (comment). This means that it introduces a counter in the feature class to detect the number of events a feature has been asked to be present while it was hidden. This allows to remove the call of the
is_present
method indoctest/control.py
.In order to test it I ran
sage -tp --all --hide=all
. Ideally this should give All tests passed. But I got two failing files. One of those wassrc/sage/features/databases.py
becausedatabase_cremona_mini_ellcurve
was detected to be optional even thought it is a standard package. This is a leftover of #35820 which I fix here.Similarily I got 37 failures in
src/sage/groups/abelian_gps/abelian_group.py
sincegap_package_polycyclic
was classified optional even though it is a Gap standard package since Gap 4.10 (as well asgap_package_atlasrep
). But in this case a corresponding fix, i.e.:is not the correct way (it gives
UserWarning: Feature gap_package_polycyclic is declared standard, but it is provided by gap_packages, which is declared optional in SAGE_ROOT/build/pkgs
). I would say, the correct fix would be to remove both Gap packages from theall_features
list and erase the corresponding optional tags inpermgroup_named.py
,distance_regular.pyx
,abelian_aut.py
,abelian_group.py
andabelian_group_gap.py
. If you agree I will open another PR for this.BTW: there seem to be more packages with ambiguous type (detected with current Docker image):
📝 Checklist
⌛ Dependencies