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

Refactor OmniSci to Heavy #454

Merged
merged 18 commits into from
Mar 23, 2022
Merged

Refactor OmniSci to Heavy #454

merged 18 commits into from
Mar 23, 2022

Conversation

tupui
Copy link
Contributor

@tupui tupui commented Mar 15, 2022

Closes #451

@tupui tupui added heavydb Related to heavydb server deprecation To deprecating something labels Mar 15, 2022
@tupui tupui self-assigned this Mar 15, 2022
@tupui tupui force-pushed the tupui/rebrand_omni_heavy branch from f411e24 to 86b2888 Compare March 15, 2022 13:54
@tupui tupui force-pushed the tupui/rebrand_omni_heavy branch 2 times, most recently from 8f2ed88 to b3d6bfe Compare March 15, 2022 17:58
@tupui tupui force-pushed the tupui/rebrand_omni_heavy branch from b3d6bfe to 4285033 Compare March 16, 2022 09:32
pearu
pearu suggested changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

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

Thanks, @tupui for this work!

I have a couple of nits.

In addition, at some point, we should filter out heavydb dependent code from test_externals_cmath.py and test_externals_libdevice.py as well and move these under tests/heavyai/

rbc/remotejit.py
@@ -19,7 +19,7 @@
# XXX WIP: the OmnisciCompilerPipeline is no longer omnisci-specific because
# we support Arrays even without omnisci, so it must be renamed and moved
# somewhere elsef
from .omnisci_backend import OmnisciCompilerPipeline
from .heavyai import OmnisciCompilerPipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

At some point (not necessarily here to keep this PR simple), we'll need to rename OmnisciCompilerPipeline as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tupui , could you open an issue for renaming OmnisciCompilerPipeline?

@pearu
Copy link
Contributor

pearu commented Mar 17, 2022

https://github.com/xnd-project/rbc/blob/master/.github/workflows/rbc_test.yml requires an update as well. For instance, currently, CI does not run any of the heavyai tests..

@tupui
Copy link
Contributor Author

tupui commented Mar 18, 2022

Most tests seem to be passing except: rbc/tests/heavyai/test_math.py::test_numpy_function[np.positive-cpu]
There is a core dump when evaluating: nb.njit(fn)

@tupui tupui requested a review from pearu March 22, 2022 07:39
@tupui
Copy link
Contributor Author

tupui commented Mar 22, 2022

@pearu do you have a hint?

@guilhermeleobas guilhermeleobas force-pushed the tupui/rebrand_omni_heavy branch from 8f5f7fa to 7293693 Compare March 22, 2022 15:05
@tupui
Copy link
Contributor Author

tupui commented Mar 22, 2022 •
edited
Loading

ok we debugged with @guilhermeleobas and #445 fixes the segfault.

@pearu Once this is green, I propose to merge maybe with the test skip, and then @guilhermeleobas would remove it in his PR.

(just seeing some other unrelated things I will fix)

@pearu
Copy link
Contributor

pearu commented Mar 22, 2022

Considering that this PR is mostly about renaming omnisci to heavydb, @tupui , @guilhermeleobas could you explain how #445 that deals with numba internals is related to the changes in this PR?

@tupui
Copy link
Contributor Author

tupui commented Mar 22, 2022

Considering that this PR is mostly about renaming omnisci to heavydb, @tupui , @guilhermeleobas could you explain how #445 that deals with numba internals is related to the changes in this PR?

AFAIU, @guilhermeleobas noticed it elsewhere.

@guilhermeleobas
Copy link
Contributor

guilhermeleobas commented Mar 22, 2022 •
edited
Loading

Considering that this PR is mostly about renaming omnisci to heavydb, @tupui , @guilhermeleobas could you explain how #445 that deals with numba internals is related to the changes in this PR?

It is a combination of factors that in the end involving Numba calling a C function that doesn't exist. I've seeing this failure before and what triggers it is running a tests that raises an exception followed by rbc/tests/heavyai/test_math.py::test_numpy_function[np.positive-cpu]. #445 solves by not generating or attempting to call the cfunc.

I had the same issue in #452

@tupui tupui marked this pull request as ready for review March 22, 2022 16:15
@tupui
Copy link
Contributor Author

tupui commented Mar 22, 2022

Ok seems like all CI is passing. There is one workflow failing one test but seems unrelated test_add_ints_rjit. I tried to relaunch the failed workflow but GitHub is having issues currently.

@tupui
Copy link
Contributor Author

tupui commented Mar 22, 2022

See here for to view the previous run: https://github.com/xnd-project/rbc/actions/runs/2023309018/attempts/1

@tupui
Copy link
Contributor Author

tupui commented Mar 22, 2022

@pearu @guilhermeleobas the CI is green. It was just a hiccup (maybe even related to GH's current outage).

pearu
pearu approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@pearu pearu 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 are a couple of discussion items that should be addressed in a follow-up but let's land this now.

Thanks, @tupui!

.github/workflows/rbc_test.yml
@@ -225,7 +225,7 @@ jobs:
EXPECTED_NUMBA_VERSION: ${{ matrix.numba-version }}
RBC_TESTS_FULL: TRUE
run: |
mamba run -n rbc pytest -sv -r A rbc/tests/ -x -k omnisci
mamba run -n rbc pytest -sv -r A rbc/tests/ -x -k heavyai
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mamba run -n rbc pytest -sv -r A rbc/tests/ -x -k heavyai
mamba run -n rbc pytest -sv -r A rbc/tests/heavyai/ -x

Copy link
Contributor

Choose a reason for hiding this comment

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

However, let's leave this change to a follow-up PR when heavydb stuff from tests/test_externals_xyz.py is moved under tests/heavyai. @tupui could you open an issue for this task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure 👍

rbc/remotejit.py
@@ -19,7 +19,7 @@
# XXX WIP: the OmnisciCompilerPipeline is no longer omnisci-specific because
# we support Arrays even without omnisci, so it must be renamed and moved
# somewhere elsef
from .omnisci_backend import OmnisciCompilerPipeline
from .heavyai import OmnisciCompilerPipeline
Copy link
Contributor

Choose a reason for hiding this comment

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

@tupui , could you open an issue for renaming OmnisciCompilerPipeline?

@tupui tupui merged commit 2044a3a into heavyai:master Mar 23, 2022
@tupui tupui deleted the tupui/rebrand_omni_heavy branch March 23, 2022 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pearu pearu pearu approved these changes

@guilhermeleobas guilhermeleobas guilhermeleobas approved these changes

Assignees

@tupui tupui

Labels
deprecation To deprecating something heavydb Related to heavydb server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rename omnisci to heavydb
3 participants
@tupui @pearu @guilhermeleobas