Fix SSL timeout in doctest / internet feature #36696
Conversation
|
Note that this is not trivial to test, since the timeout bug is hard to reproduce (it triggers only if the timeout happens after the https request is made and before the response is read back, or something like that). A reasonable way to test the second commit is to temporarily patch the method |
|
Are the following failures my fault? |
|
I'll assume is some of this shit: Heck, these are moving targets and the log doesn't even print the commit hash that was merged! Bear in mind this PR was pushed within a day of 10.2.rc1 being released. #36372 goes to show how broken this act is: there is a disagreement on the status of that PR, and CI is being hold hostage. |
No, they also show up in other PRs. A problem with nauty. |
I don't think so.
Actually the log does print all that information, just not where you looked. https://github.com/sagemath/sage/actions/runs/6830550892/job/18578658840?pr=36696#step:3:20 The applied patches are also in the "upstream" artifact - https://github.com/sagemath/sage/actions/runs/6830550892?pr=36696 |
If you mean this: There's not enough information here. These are the merge commits, but I don't expect they will be anywhere I can find them (i.e., in the PR). Usually when I do At least this way I can see 3f92c49 is the commit that was merged, and I can find it in the PR (in fact it's this PR). Let's compare with 3d28c298df87999cb38a8b94aa05270aaff0550f <--- see, not even GH knows about this commit.
That's so much friction that I'm already sweating (no good since summer is coming here in the south). Also, I worry that different jobs may be started at different times; so it could be that different jobs in the same check merge different commits (e.g. in case some blocker pr changes in between). For the same reason it'd be nice to print the same (complete) information in every job, and don't print incomplete information anywhere. If showing the
which would be hopefully clickable (needs the full URL since only complete URLs are clickable in logs afaik). Or else, just print a link to the PR + link to the commit instead of only a link to the PR. My constructive suggestion above should not be confused with me endorsing this scheme, which I still think it's too brittle (both from technical and social sides). |
CI seems to be fixed now (see #36702). Shall I then restart this check? (only way I know is to amend and force push). |
tornaria
force-pushed
the
ssl_timeout
branch
from
3f92c49 to
4a70c1e
Compare
No, I think it's a stochastically appearing problem. My current guess is that the recently updated nauty does not respect SAGE_FAT_BINARY and thus it depends on CPU details of the runner |
You are right.
OK, I'll see what I can do.
I agree, it's far from convenient.
Good idea and easy to do. (Edit: I'll do this in #36686) |
Ah, that's easy to fix: add |
I mean possibly. I didn't test. Also note |
|
Thanks a lot! Happening in #36707 |
|
Not sure if your change to diff --git a/src/sage/doctest/control.py b/src/sage/doctest/control.py
index 8583d7a447d..297da6133ce 100644
--- a/src/sage/doctest/control.py
+++ b/src/sage/doctest/control.py
@@ -1502,18 +1502,14 @@ class DocTestController(SageObject):
available_software._allow_external = self.options.optional is True or 'external' in self.options.optional
for h in self.options.hide:
- try:
+ if h in available_software:
i = available_software._indices[h]
- except KeyError:
- pass
- else:
f = available_software._features[i]
- if f.is_present():
- f.hide()
- self.options.hidden_features.add(f)
- for g in f.joined_features():
- if g.name in self.options.optional:
- self.options.optional.discard(g.name)
+ f.hide()
+ self.options.hidden_features.add(f)
+ for g in f.joined_features():
+ if g.name in self.options.optional:
+ self.options.optional.discard(g.name)
for o in self.options.disabled_optional:
try: |
Indeed, that is the problema, and this solution feels much better. I've replaced my commit by yours. |
tornaria
force-pushed
the
ssl_timeout
branch
from
4a70c1e to
ef73c85
Compare
The logic is not working properly. It seems meataxe is not being hide (in my local test this didn't hit me because I don't have meataxe installed). IOW, I think the reason this gives "5 tests" instead of "4 tests" is because it's actually running the last test (labeled "optional meataxe") in spite of the hidden meataxe. It's possible that it's another bug so that calling I'm inclined to revert to my previous workaround, since this looks like a slippery slope (cf. a series of unfortunate events vol 10). |
That would be fine with me for getting 10.2 out |
|
Downgrading until fixed |
tornaria
force-pushed
the
ssl_timeout
branch
from
ef73c85 to
4a70c1e
Compare
That feels a bit rude. What is the point? I've reverted the branch to 4a70c1e which is what was tested before. |
|
Documentation preview for this PR (built with commit 4a70c1e; changes) is ready! 🎉 |
Sorry, just so that the doctest failure doesn't show up in the CI runs of all PRs. |
mkoeppe
added
s: positive review
p: blocker / 1
and removed
s: needs review
p: critical / 2
labels
|
Let's get this in. |
I assumed "blocker" without "positive review" is not automatically merged, is it? If it is, you did the right thing here of course, but it kind of violates the principle of least surprise. Thanks for the review. |
It is, actually. |
That's a problem, because when you do something completely correct from a technical point of view, it could still feel a little bit rude from a social point of view, specially if the person involved is not aware of the reason. Wouldn't it make sense to have a separate label for "hotfixes" ? Although maybe having a "hotfix" branch (with some person in charge) would be better. Maybe call it |
I agree, so the best practice should be to always include a clear comment when relabeling. I've updated https://github.com/sagemath/sage/wiki/Sage-10.2-Release-Tour#open-blocker-prs-are-applied-automatically-in-ci-workflows, take a look |
Recently I've been getting timeouts in a couple of doctests in
`src/sage/doctest/control.py`, including earlier today when testing
10.2.rc1
This has two causes:
1. The test for the internet feature doesn't catch a `TimeoutError`.
2. These two doctests cause internet feature to be tested.
The first one is clearly a bug: sagemath only expects
`urllib.error.URLError`, but in some cases urllib raises `TimeoutError`.
Arguably a bug in urlib, but easier to fix (or workaround) on our side.
Done in the first commit of this PR.
The second one is IMO a bug, since I didn't use `--optional=internet` no
test should hit internet (in particular, should not test internet
feature).
These two doctests are meant to test options `--hide=all` and
`--hide=optional`. For some reason the semantics of these include
testing for the internet feature. I believe the semantics of these
options should be similar to `--optional=all`, that is, exclude
"external" software (internet feature is considered "external").
That's what I implemented in the second commit in this PR.
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
URL: sagemath#36696
Reported by: Gonzalo Tornaría
Reviewer(s):
|
Sorry I wasn't able to attend at the weekend.
Yes! The sole purpose of calling |
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> Handling `SAGE_FAT_BINARY` as suggested by @tornaria in sagemath#36696 (comment) Disabling popcnt is consistent with the handling of SAGE_FAT_BINARY in our `primesieve` install script. Bumping the patch level so it gets recompiled when applied as a CI hotfix. <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [ ] 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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36707 Reported by: Matthias Köppe Reviewer(s): Dima Pasechnik
This is #36741! |
…customizable by repository variable
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
Instead of using `gh pr checkout`, we obtain the CI fixes via their
patch URLs.
- This is faster because typically we do not have to unshallow the repo
to apply the patches (seconds instead of ~2 minutes)
- Fewer surprises when applied to a PR based on an older release
- Conjecturally uses fewer API queries, helping avoid sagemath#36685
When a repository variable `SAGE_CI_FIXES_FROM_REPOSITORIES` is set in a
fork, it is used instead of the hardcoded sagemath/sage as the source(s)
of the CI fixes; this gives better control in decentralized development.
When set to "none", this also makes it possible to see the "ground
truth", addressing a concern raised in
sagemath#36349 (comment). See
comments at the top of `.ci/merge-fixes.sh` for details.
Also improving the display of details of the merged PRs, as requested by
@tornaria in
sagemath#36696 (comment)
Example runs:
- https://github.com/sagemath/sage/actions/runs/6854931832/job/186389018
59?pr=36686 (here it recovers gracefully from failed merges due to the
10.2.rc1/10.2.rc2 tagging confusions)
- https://github.com/mkoeppe/sage/actions/runs/6855410957/job/1864038039
8#step:3:8 (I have set `SAGE_CI_FIXES_FROM_REPOSITORIES=none` in my fork
-- for testing this PR)
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [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 accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36686
Reported by: Matthias Köppe
Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe
…customizable by repository variable
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
Instead of using `gh pr checkout`, we obtain the CI fixes via their
patch URLs.
- This is faster because typically we do not have to unshallow the repo
to apply the patches (seconds instead of ~2 minutes)
- Fewer surprises when applied to a PR based on an older release
- Conjecturally uses fewer API queries, helping avoid sagemath#36685
When a repository variable `SAGE_CI_FIXES_FROM_REPOSITORIES` is set in a
fork, it is used instead of the hardcoded sagemath/sage as the source(s)
of the CI fixes; this gives better control in decentralized development.
When set to "none", this also makes it possible to see the "ground
truth", addressing a concern raised in
sagemath#36349 (comment). See
comments at the top of `.ci/merge-fixes.sh` for details.
Also improving the display of details of the merged PRs, as requested by
@tornaria in
sagemath#36696 (comment)
Example runs:
- https://github.com/sagemath/sage/actions/runs/6854931832/job/186389018
59?pr=36686 (here it recovers gracefully from failed merges due to the
10.2.rc1/10.2.rc2 tagging confusions)
- https://github.com/mkoeppe/sage/actions/runs/6855410957/job/1864038039
8#step:3:8 (I have set `SAGE_CI_FIXES_FROM_REPOSITORIES=none` in my fork
-- for testing this PR)
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [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 accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36686
Reported by: Matthias Köppe
Reviewer(s): Gonzalo Tornaría, Kwankyu Lee, Matthias Köppe
…kages
<!-- ^^^^^
Please provide a concise, informative and self-explanatory title.
Don't put issue numbers in there, do this in the PR body below.
For example, instead of "Fixes sagemath#1234" use "Introduce new method to
calculate 1+1"
-->
<!-- Describe your changes here in detail -->
<!-- Why is this change required? What problem does it solve? -->
<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes sagemath#12345". -->
<!-- If your change requires a documentation PR, please link it
appropriately. -->
This PR implements the suggestion made in
sagemath#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 in
`doctest/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 was `src/sage/features/databases.py` because
`database_cremona_mini_ellcurve` was detected to be optional even
thought it is a standard package. This is a leftover of
sagemath#35820 which I fix here.
Similarily I got 37 failures in
`src/sage/groups/abelian_gps/abelian_group.py` since
`gap_package_polycyclic` was classified optional even though it is a Gap
standard package since Gap 4.10 (as well as `gap_package_atlasrep`). But
in this case a corresponding fix, i.e.:
```
def all_features():
- return [GapPackage("atlasrep", spkg="gap_packages"),
+ return [GapPackage("atlasrep", spkg="gap_packages",
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_packages",
type='standard'),
GapPackage("qpa", spkg="gap_packages"),
GapPackage("quagroup", spkg="gap_packages")]
```
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 the
`all_features` list and erase the corresponding *optional tags* in
`permgroup_named.py`, `distance_regular.pyx`, `abelian_aut.py`,
`abelian_group.py` and `abelian_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):
```
Digest:
sha256:790197bb223bd7e20b0a2e055aa7b4c5846dc86d94b2425cd233cb6160a5b944
Status: Downloaded newer image for sagemath/sagemath:develop
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.2.rc4, Release Date: 2023-11-17 │
│ Using Python 3.11.1. Type "help()" for help. │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable. ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: from sage.features.all import all_features
sage: [(f, f._spkg_type()) for f in all_features() if f.is_present() and
f._spkg_type() != 'standard']
[(Feature('sage.misc.cython'), 'optional'),
(Feature('database_cremona_mini_ellcurve': Cremona's database of
elliptic curves),
'optional'),
(Feature('gap_package_atlasrep'), 'optional'),
(Feature('gap_package_polycyclic'), 'optional'),
(Feature('sage.libs.ecl'), 'optional'),
(Feature('sage.libs.gap'), 'optional'),
(Feature('sage.libs.singular'), 'optional'),
(Feature('sage.numerical.mip'), 'optional')]
sage:
sage: [(f, f._spkg_type()) for f in all_features() if not f.is_present()
and f._spkg_type() == 'standard']
[(Feature('sagemath_doc_html'), 'standard'), (Feature('sage.sat'),
'standard')]
```
### 📝 Checklist
<!-- Put an `x` in all the boxes that apply. -->
<!-- If your change requires a documentation PR, please link it
appropriately -->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
<!-- Feel free to remove irrelevant items. -->
- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [x] I have created tests covering the changes.
- [x] I have updated the documentation accordingly.
### ⌛ Dependencies
<!-- List all open PRs that this PR logically depends on
- sagemath#12345: short description why this is a dependency
- sagemath#34567: ...
-->
<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
URL: sagemath#36741
Reported by: Sebastian Oehms
Reviewer(s): Matthias Köppe, Sebastian Oehms
Recently I've been getting timeouts in a couple of doctests in
src/sage/doctest/control.py, including earlier today when testing 10.2.rc1This has two causes:
TimeoutError.The first one is clearly a bug: sagemath only expects
urllib.error.URLError, but in some cases urllib raisesTimeoutError. Arguably a bug in urlib, but easier to fix (or workaround) on our side. Done in the first commit of this PR.The second one is IMO a bug, since I didn't use
--optional=internetno test should hit internet (in particular, should not test internet feature).These two doctests are meant to test options
--hide=alland--hide=optional. For some reason the semantics of these include testing for the internet feature. I believe the semantics of these options should be similar to--optional=all, that is, exclude "external" software (internet feature is considered "external").That's what I implemented in the second commit in this PR.