-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add co_stacksize to code objects #4554
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
Conversation
I need to remind myself to update |
wow, this makes a lot of tests pass 😄 The following tests in test_async_for (test.test_compile.TestStackSizeStability) ... unexpected success |
Heya @howjmay, if there's confusion on how to do it or you don't have enough time to do so let me know and I'll fix the tests as needed. |
Hi @DimitrisJim I was busy in the past week. I am starting to fix it once again |
@DimitrisJim I am feel sorry that I still can't solve this. I have no idea how to get the correct stack size |
@howjmay try going into # TODO: RUSTPYTHON
@unittest.expectedFailure |
your previous commit (c80802d) was perfectly fine here! Like @fanninpm mentions, just go through the tests in If the stack depth is calculated incorrectly in some cases, that's another subject. This PR just takes care of exposing it for now so don't worry about it. |
@fanninpm @DimitrisJim Ah thanks! I was totally misunderstood. Let me fix it! |
yeah, sorry if I didn't make myself clear enough. The idea here is to remove any decorators (and their preceding comment) for the tests that now pass. If you revert the changes you've made to cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v test_compile this will execute the test runner for this specific file. After you do that you'll notice that for some of the functions (the ones in #4554 (comment)) the test actually passed (there will be a message saying 'Unexpected success'). For these you'll just need to remove the Hope I made it clear! Feel free to ask if something is confusing. |
Thank you @DimitrisJim. I just pushed a commit to fix |
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 @howjmay! Hopefully this now satisfies the test runner.
Thanks! |
Closes #4545