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.

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

Removes context.compile_internal where easy #8493

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

guilhermeleobas
Copy link
Collaborator

@guilhermeleobas guilhermeleobas commented Oct 6, 2022

Removes context.compile_internal where easy

  • np.ravel
  • array.flatten
  • array.sort
  • round
  • not in
  • math.gcd
  • np.argsort

Copy link
Collaborator

@kc611 kc611 left a comment

Choose a reason for hiding this comment

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

Looks Good to me!

Few minor nitpicks below.

numba/np/arrayobj.py Outdated Show resolved Hide resolved
numba/np/arrayobj.py Show resolved Hide resolved
numba/np/arrayobj.py Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Oct 10, 2022
@stuartarchibald stuartarchibald added this to the Numba 0.57 RC milestone Oct 10, 2022
@stuartarchibald stuartarchibald added the Effort - medium Medium size effort needed label Oct 10, 2022
@guilhermeleobas guilhermeleobas 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 Oct 10, 2022
@guilhermeleobas
Copy link
Collaborator Author

Hi @kc611, thanks for the review. I have addressed all your comments.

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

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

Thanks for doing this refactoring @guilhermeleobas. A couple of comments to resolve else looks good.

numba/cpython/builtins.py Outdated Show resolved Hide resolved
numba/np/arrayobj.py Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald 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 Oct 11, 2022
gmarkall
gmarkall previously approved these changes Oct 31, 2022
Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Thanks for the update - this looks good to me from a CUDA perspective now.

@gmarkall
Copy link
Member

(Note that the gpuCI fail is just an issue inherited from main, not this PR)

@gmarkall
Copy link
Member

gmarkall commented Nov 1, 2022

gpuci run tests

(just to make everything look nice and green now #8537 is merged)

@stuartarchibald
Copy link
Contributor

@guilhermeleobas please could you resolve the conflicts against main? Many thanks.

@stuartarchibald stuartarchibald 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 Nov 8, 2022
@guilhermeleobas
Copy link
Collaborator Author

I'm seeing some failures in this PR after migrating np.ravel to @overload:
https://gist.github.com/guilhermeleobas/91fc7a4137b4fa27b95ff4b8830d0952

@guilhermeleobas guilhermeleobas 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 Nov 11, 2022
@stuartarchibald
Copy link
Contributor

I'm seeing some failures in this PR after migrating np.ravel to @overload: https://gist.github.com/guilhermeleobas/91fc7a4137b4fa27b95ff4b8830d0952

Public CI is passing, is this still an issue or was it resolved by the latest two commits (or just missed by public CI)?

@guilhermeleobas
Copy link
Collaborator Author

I'm seeing some failures in this PR after migrating np.ravel to @overload: https://gist.github.com/guilhermeleobas/91fc7a4137b4fa27b95ff4b8830d0952

Public CI is passing, is this still an issue or was it resolved by the latest two commits (or just missed by public CI)?

This is still an "issue".

@gmarkall
Copy link
Member

This message in the traceback: Only accept returning of array passed into the function as argument I see popping up when NRT is not available (usually in the CUDA target, but these messages indicate nopython mode) - is there something disabling NRT going on there?

@stuartarchibald
Copy link
Contributor

This message in the traceback: Only accept returning of array passed into the function as argument I see popping up when NRT is not available (usually in the CUDA target, but these messages indicate nopython mode) - is there something disabling NRT going on there?

I think this is indeed the root cause of the problem. In the test this is present:

def test_ravel_array(self, flags=enable_pyobj_flags):

which is set via this:
enable_pyobj_flags = Flags()
enable_pyobj_flags.enable_pyobject = True

The Flags() ctor won't enable the NRT by default.

This problem exists on main too, it's just far less "noisy" as the split typing/lowering API doesn't have such tracebacks. I think the test needs updating to run with the NRT enabled.

@stuartarchibald stuartarchibald 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 Nov 30, 2022
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

This pull request is marked as stale as it has had no activity in the past 3 months. Please respond to this comment if you're still interested in working on this. Many thanks!

@github-actions github-actions bot added the stale Marker label for stale issues. label Mar 1, 2023
@github-actions github-actions bot added the abandoned - stale PRs automatically closed due to stale status label Mar 9, 2023
@github-actions github-actions bot closed this Mar 9, 2023
@guilhermeleobas
Copy link
Collaborator Author

I should probably give another try to this PR sometime in the future.

@guilhermeleobas guilhermeleobas added 2 - In Progress and removed abandoned - stale PRs automatically closed due to stale status stale Marker label for stale issues. 4 - Waiting on author Waiting for author to respond to review labels Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants