Skip to content

Conversation

zooba
Copy link
Member

@zooba zooba commented Dec 9, 2021

@mdickinson
Copy link
Member

mdickinson commented Dec 9, 2021

Just to be clear, was the bogus OverflowError witnessed on an actual system, or is this purely a hypothetical possibility? I'm assuming the former, but it wasn't totally clear from the description.

I'd propose a simpler fix: replace the line

if (fabs(x) < 1.0)

with

        if (fabs(x) < 2.0)

It's arguably more dangerous in that the change potentially affects all functions using is_error instead of just changing the behaviour of expm1, but I think it's safe in practice: we only need to distinguish underflow from overflow, and 2.0 is just as good a threshold as 1.0 for that.

Please could you also add a testcase that would exercise the problematic case, if there isn't one already?

@zooba
Copy link
Member Author

zooba commented Dec 9, 2021

Just to be clear, was the bogus OverflowError witnessed on an actual system, or is this purely a hypothetical possibility?

It was an actual prerelease system, and it's entirely possible that the library will be fixed before it ever sees real use. But logically there's no way for expm1 to underflow to 0.0 anyway - it will always go to -1.0, because that's the spec (expm1(x) == exp(x) - 1.0).

I expanded the range to 1.5, as there's really no need to go any further. Quite likely making it <= 1.0 would be sufficient, but why risk it.

@mdickinson
Copy link
Member

Please could you also add a testcase that would exercise the problematic case, if there isn't one already?

Update: no need - it's already covered. (See discussion on the b.p.o. issue.)

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mdickinson
Copy link
Member

I guess it's worth running the buildbots on this one, just in case.

@mdickinson mdickinson added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 9, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @mdickinson for commit c778229 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 9, 2021
@zooba
Copy link
Member Author

zooba commented Dec 9, 2021

The SLES buildbot failed because it timed out downloading Unicode files from an external server. Definitely not related to this change.

@mdickinson
Copy link
Member

The buildbots seem happy so far, apart from the slow ones ... I don't think we need to wait for the rest of the buildbot results before merging - I'm happy to keep an eye on the remaining ones and raise an alert in the unlikely event that any of them fails for reasons related to this PR.

@mdickinson
Copy link
Member

If we're regarding this as a kinda/sorta bugfix, should it be backported to 3.10 and 3.9?

@zooba zooba added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Dec 9, 2021
@zooba
Copy link
Member Author

zooba commented Dec 9, 2021

Yes, I think it should be backported. I'll keep an eye on buildbots as well, but I expect this one to be pretty innocuous.

@zooba zooba merged commit 3363e1c into python:main Dec 9, 2021
@miss-islington
Copy link
Contributor

Thanks @zooba for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

@zooba zooba deleted the bpo-46018 branch December 9, 2021 18:31
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 9, 2021
…GH-29997)

(cherry picked from commit 3363e1c)

Co-authored-by: Steve Dower <steve.dower@python.org>
@bedevere-bot
Copy link

GH-30012 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Dec 9, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 9, 2021
…GH-29997)

(cherry picked from commit 3363e1c)

Co-authored-by: Steve Dower <steve.dower@python.org>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Dec 9, 2021
@bedevere-bot
Copy link

GH-30013 is a backport of this pull request to the 3.9 branch.

miss-islington added a commit that referenced this pull request Dec 9, 2021
(cherry picked from commit 3363e1c)

Co-authored-by: Steve Dower <steve.dower@python.org>
miss-islington added a commit that referenced this pull request Dec 9, 2021
(cherry picked from commit 3363e1c)

Co-authored-by: Steve Dower <steve.dower@python.org>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL7 LTO + PGO 3.10 has failed when building commit ca08655.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/663/builds/534) and take a look at the build logs.
  4. Check if the failure is related to this commit (ca08655) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/663/builds/534

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

411 tests OK.

10 slowest tests:

  • test_concurrent_futures: 2 min 42 sec
  • test_multiprocessing_spawn: 2 min 8 sec
  • test_multiprocessing_forkserver: 1 min 41 sec
  • test_tokenize: 1 min 24 sec
  • test_multiprocessing_fork: 1 min 22 sec
  • test_unparse: 1 min 15 sec
  • test_lib2to3: 1 min 4 sec
  • test_asyncio: 54.2 sec
  • test_signal: 48.0 sec
  • test_socket: 44.1 sec

1 test altered the execution environment:
test_asyncio

15 tests skipped:
test_devpoll test_gdb test_ioctl test_kqueue test_msilib
test_smtpnet test_ssl test_startfile test_tix test_tk
test_ttk_guionly test_winconsoleio test_winreg test_winsound
test_zipfile64

Total duration: 6 min 51 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.10.edelsohn-rhel-z.lto-pgo/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
KeyError: '/psm_802bfe0d'


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.10.edelsohn-rhel-z.lto-pgo/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
KeyError: '/psm_2c667e76'


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.10.edelsohn-rhel-z.lto-pgo/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
KeyError: '/psm_089904f0'

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.

5 participants