Skip to content

Drop pre-3.7 code path (CPU only) - #7920

Merged
sklam merged 11 commits into
numba:mainfrom
sklam:enh/drop_pre_byteflow
Sep 20, 2022
Merged

Drop pre-3.7 code path (CPU only)#7920
sklam merged 11 commits into
numba:mainfrom
sklam:enh/drop_pre_byteflow

Conversation

@sklam

@sklam sklam commented Mar 16, 2022

Copy link
Copy Markdown
Member

Intentionally separating work for non-CPU code path.

@sklam
sklam marked this pull request as ready for review March 21, 2022 14:51
@stuartarchibald

stuartarchibald commented Mar 30, 2022

Copy link
Copy Markdown
Contributor

@sklam is this intended for 0.57?

@sklam

sklam commented Mar 31, 2022

Copy link
Copy Markdown
Member Author

@sklam is this intended for 0.57?

Sure. We already dropped py<3.7.

@stuartarchibald stuartarchibald added this to the Numba 0.57 RC milestone Apr 1, 2022
@stuartarchibald stuartarchibald added the Effort - medium Medium size effort needed label Apr 1, 2022
@esc
esc removed the request for review from stuartarchibald July 12, 2022 14:56
@jamesobutler jamesobutler mentioned this pull request Aug 7, 2022
Closed
@esc esc mentioned this pull request Aug 12, 2022
Merged
esc
esc requested changes Aug 15, 2022
View reviewed changes

@esc esc left a comment •
edited
Loading

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.

I compared #7920 and #8318 today. It turns out, that neither is complete and both contain changes not present in the other. Also, they can't be merged into each other due to conflicts..

Moving forward, I suggest @sklam picking the following changes:

https://github.com/numba/numba/pull/8318/files#diff-2df48118bba9e73a681d271761e063d5f84ea485bd00b65c7afd7799fba930c7L1107-R1114

https://github.com/numba/numba/pull/8318/files#diff-bf52cc8cc7ef5a7f153acd1c35784daeba459c55aff21631e7d399b83e65ee5cL2128-R2159

https://github.com/numba/numba/pull/8318/files#diff-b925aa196d8a9aa356e639fb4af8049c9ad14ebb16d8f71b9df2d976d46a29c2L4-R181

@esc esc added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Aug 15, 2022
jamesobutler
jamesobutler reviewed Aug 18, 2022
View reviewed changes

@jamesobutler jamesobutler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes my changes that were brought over here appear to make the coverage of dropping all pre-3.7 code complete 👍🏻

@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Aug 18, 2022
@esc

esc commented Aug 19, 2022

Copy link
Copy Markdown
Member

@sklam thank you for fixing this up! @jamesobutler thank you for your review. I tried a few git grep combinations locally and only found two potentially remaining instances

This is comment, but it seems like it is a "dead comment" now:

https://github.com/numba/numba/blob/main/numba/tests/test_withlifting.py#L1109-L1110

This could be made more precise by using == (3, 7):

https://github.com/numba/numba/blob/main/numba/tests/test_withlifting.py#L767

@esc esc added 4 - Waiting on author Waiting for author to respond to review and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Aug 29, 2022
@sklam

sklam commented Sep 13, 2022

Copy link
Copy Markdown
Member Author

This is comment, but it seems like it is a "dead comment" now:

https://github.com/numba/numba/blob/main/numba/tests/test_withlifting.py#L1109-L1110

changed in d2d76a6

This could be made more precise by using == (3, 7):

https://github.com/numba/numba/blob/main/numba/tests/test_withlifting.py#L767

I prefer to leave it as <= (3, 7). I consider that as defensive programming

@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Sep 13, 2022
@sklam
sklam requested a review from esc September 13, 2022 15:10
@esc

esc commented Sep 14, 2022

Copy link
Copy Markdown
Member

This is comment, but it seems like it is a "dead comment" now:
https://github.com/numba/numba/blob/main/numba/tests/test_withlifting.py#L1109-L1110

changed in d2d76a6

Thank you

This could be made more precise by using == (3, 7):
https://github.com/numba/numba/blob/main/numba/tests/test_withlifting.py#L767

I prefer to leave it as <= (3, 7). I consider that as defensive programming

ok, that makes sense.

@esc

esc commented Sep 14, 2022

Copy link
Copy Markdown
Member

Build numba_smoketest_cpu_yaml_130 has started

@esc esc added the Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm label Sep 14, 2022
@esc

esc commented Sep 16, 2022

Copy link
Copy Markdown
Member

Build numba_smoketest_cpu_yaml_130 has started

Has passed fine.

@esc esc added BuildFarm Passed For PRs that have been through the buildfarm and passed 5 - Ready to merge Review and testing done, is ready to merge and removed Pending BuildFarm For PRs that have been reviewed but pending a push through our buildfarm 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Sep 16, 2022
@sklam

sklam commented Sep 19, 2022

Copy link
Copy Markdown
Member Author

@esc, can you approve this PR please?

esc
esc approved these changes Sep 20, 2022
View reviewed changes

@esc esc 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.

LGTM

@esc

esc commented Sep 20, 2022

Copy link
Copy Markdown
Member

@esc, can you approve this PR please?

done.

@sklam
sklam merged commit 5036115 into numba:main Sep 20, 2022
@sklam
sklam deleted the enh/drop_pre_byteflow branch September 20, 2022 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@esc esc esc approved these changes
+1 more reviewer
@jamesobutler jamesobutler jamesobutler left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@esc esc

Labels

5 - Ready to merge Review and testing done, is ready to merge BuildFarm Passed For PRs that have been through the buildfarm and passed Effort - medium Medium size effort needed

Projects

None yet

Milestone

Numba 0.57 RC

Development

Successfully merging this pull request may close these issues.

4 participants

@sklam @stuartarchibald @esc @jamesobutler