Skip to content

Make Python 3.10 kwarg peephole less restrictive - #8044

Merged
sklam merged 3 commits into
numba:mainfrom
njriasan:nick/fix_unnecessary_check
May 16, 2022
Merged

sklam merged 3 commits into
numba:mainfrom
njriasan:nick/fix_unnecessary_check

Conversation

@njriasan

Copy link
Copy Markdown
Contributor

Closes #8043

Fixes a bug in peep_hole_call_function_ex_to_call_function_kw where we raised the error if we couldn't find the varargs in the basic block and no kwargs are passed. This is an inaccurate check and we should only raise an exception if we find the args have the wrong definition.

This also fixes a bug in peep_hole_list_to_tuple where the definition of the final assignment wasn't properly set.

@sklam

sklam commented May 12, 2022

Copy link
Copy Markdown
Member

I did a quick check. It does resolve the failure in numba.tests.test_ndarray_subclasses.TestNdarraySubclasses

@sklam

sklam commented May 12, 2022

Copy link
Copy Markdown
Member

smoketest BFID: numba_smoketest_cpu_yaml_83

@esc esc added 4 - Waiting on reviewer Waiting for reviewer to respond to author 4 - Waiting on CI Review etc done, waiting for CI to finish labels May 13, 2022
@stuartarchibald stuartarchibald added this to the Numba 0.56 RC milestone May 13, 2022
@stuartarchibald

stuartarchibald commented May 13, 2022 •

Copy link
Copy Markdown
Contributor

smoketest BFID: numba_smoketest_cpu_yaml_83

This passed on multiple build jobs with Python 3.10, and all other build jobs not impacted by some other unrelated issue.

Just checked into the few failing Python 3.10 jobs, there's still a couple that are reporting the issue in #8043. Need to take a closer look as to what has happened.

@njriasan

Copy link
Copy Markdown
Contributor Author

smoketest BFID: numba_smoketest_cpu_yaml_83

This passed on multiple build jobs with Python 3.10, and all other build jobs not impacted by some other unrelated issue.

Just checked into the few failing Python 3.10 jobs, there's still a couple that are reporting the issue in #8043. Need to take a closer look as to what has happened.

@stuartarchibald If you post the list of tests I'm happy to take a look.

@stuartarchibald

Copy link
Copy Markdown
Contributor

@stuartarchibald If you post the list of tests I'm happy to take a look.

Thanks @njriasan it's these two tests, same failures as in #8043:

  • numba.tests.test_ndarray_subclasses.TestNdarraySubclasses.test_my_array_return
  • numba.tests.test_ndarray_subclasses.TestNdarraySubclasses.test_my_array_allocator_override

What I'm not sure about is if this is caused by a genuine issue with this patch or there's something unusual going on in the farm builds.

Set up was Python 3.10, np 1.21, linux 64 bit system.

@njriasan

Copy link
Copy Markdown
Contributor Author

Thanks @stuartarchibald I couldn't reproduce locally on my M1, so I'm install docker to test with a linux 64 bit system. Will update after I've tested that.

@njriasan

Copy link
Copy Markdown
Contributor Author

@stuartarchibald I reran the whole test file manually using a ubuntu x86 docker image. I could not reproduce any failures. I'm inclined to believe this may be a build farm issue.

@sklam

sklam commented May 13, 2022

Copy link
Copy Markdown
Member

@stuartarchibald, I think we might be mixing up the result of two builds. I'm going to combine the two PRs (#8044 and #8046) to tests.

@stuartarchibald

Copy link
Copy Markdown
Contributor

@stuartarchibald, I think we might be mixing up the result of two builds. I'm going to combine the two PRs (#8044 and #8046) to tests.

Thanks @sklam

@stuartarchibald

Copy link
Copy Markdown
Contributor

@stuartarchibald I reran the whole test file manually using a ubuntu x86 docker image. I could not reproduce any failures. I'm inclined to believe this may be a build farm issue.

@njriasan Thanks for checking.

@sklam

sklam commented May 16, 2022

Copy link
Copy Markdown
Member

tested as part of #8053 and its all green

@sklam sklam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the patch!

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author 4 - Waiting on CI Review etc done, waiting for CI to finish labels May 16, 2022
@sklam
sklam merged commit 98db52f into numba:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python 3.10 kwarg peephole rewrite failing in valid case (breaks mainline)

4 participants