Skip to content
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.

Sign up for GitHub

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

Jump to bottom

gh-90473: Decrease recursion limit and skip tests on WASI (GH-92803) #92803

Merged
merged 7 commits into from
May 19, 2022
Merged

gh-90473: Decrease recursion limit and skip tests on WASI (GH-92803) #92803

merged 7 commits into from
May 19, 2022

Conversation

tiran
Copy link
Member

@tiran tiran commented May 14, 2022

  • Reduce recursion limit to 750. WASI has limited call stack.
  • Mark tests that require mmap, os.pipe, or fail on musl libc.

@tiran tiran force-pushed the gh-90473-wasi-rec-test branch 4 times, most recently from 0d3f889 to 24ff31d Compare May 15, 2022 13:25
@tiran tiran marked this pull request as ready for review May 15, 2022 13:25
@tiran tiran force-pushed the gh-90473-wasi-rec-test branch from 24ff31d to 1dfb80b Compare May 16, 2022 08:36
ericsnowcurrently
ericsnowcurrently reviewed May 16, 2022
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

mostly LGTM

What's the plan for un-skipping all those tests? Some of them seem like they should be fixable right now but I expect some may not ever work.

@tiran tiran force-pushed the gh-90473-wasi-rec-test branch from 1dfb80b to 6fb0e9f Compare May 17, 2022 12:29
@tiran
Copy link
Member Author

tiran commented May 17, 2022

What's the plan for un-skipping all those tests? Some of them seem like they should be fixable right now but I expect some may not ever work.

We need to revise the skips whenever we update to a new version of WASI SDK. The standard evolves in small incremental steps. Each version adds a few new features.

ericsnowcurrently
ericsnowcurrently approved these changes May 17, 2022
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM (There were a few things I'm not familiar with that I'll trust you on. 😄)

I noted a couple cases where it seems like we should be able to tweak tests instead of skipping them. However, I'll leave their resolution up to your judgement.

Lib/test/test_syntax.py Outdated
@@ -2117,6 +2117,7 @@ def test_syntax_error_on_deeply_nested_blocks(self):
self._check_error(source, "too many statically nested blocks")

@support.cpython_only
@unittest.skipIf(support.is_wasi, "Exhausts WASI call stack")
Copy link
Member

Choose a reason for hiding this comment

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

Is there some way we could tweak the test to get the same coverage?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to make the test pass with a smaller parser MAXSTACK value on WASI.

CC @pablogsal

@tiran tiran force-pushed the gh-90473-wasi-rec-test branch from 6fb0e9f to fdd80e9 Compare May 18, 2022 08:17
vstinner
vstinner approved these changes May 18, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, overall. I have a few minor remarks/suggestions.

@tiran tiran force-pushed the gh-90473-wasi-rec-test branch from 71714f4 to e67eab4 Compare May 18, 2022 13:36
pablogsal
pablogsal approved these changes May 19, 2022
View reviewed changes
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

Parser and AST changes LGTM

@tiran tiran changed the title gh-90473: Decrease recursion limit and skip tests on WASI gh-90473: Decrease recursion limit and skip tests on WASI (GH-92803) May 19, 2022
@tiran tiran merged commit 137fd3d into python:main May 19, 2022
@tiran tiran deleted the gh-90473-wasi-rec-test branch May 19, 2022 10:43
@miss-islington
Copy link
Contributor

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-92952 is a backport of this pull request to the 3.11 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 19, 2022
@tiran @miss-islington
pythongh-90473: Decrease recursion limit and skip tests on WASI (pyth…
f7b8e3a 
…onGH-92803)

(cherry picked from commit 137fd3d)

Co-authored-by: Christian Heimes <christian@python.org>
miss-islington added a commit that referenced this pull request May 19, 2022
@miss-islington @tiran
gh-90473: Decrease recursion limit and skip tests on WASI (GH-92803)
7afccd3 
(cherry picked from commit 137fd3d)

Co-authored-by: Christian Heimes <christian@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@Lesmiscore Lesmiscore Lesmiscore left review comments

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@pablogsal pablogsal pablogsal approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

@brettcannon brettcannon Awaiting requested review from brettcannon brettcannon is a code owner

@encukou encukou Awaiting requested review from encukou encukou is a code owner

@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner

@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner

@vsajip vsajip Awaiting requested review from vsajip vsajip is a code owner

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tiran @miss-islington @bedevere-bot @vstinner @ericsnowcurrently @Lesmiscore @pablogsal