FIX-#3774: Fix segfault at init if only omnisci present #3783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3783 +/- ##
==========================================
+ Coverage 85.38% 89.16% +3.78%
==========================================
Files 197 198 +1
Lines 16427 17631 +1204
==========================================
+ Hits 14026 15721 +1695
+ Misses 2401 1910 -491
Continue to review full report at Codecov.
|
|
|
|
YarShev
force-pushed
the
dev/yigoshev-issue3774
branch
from
6b3998d
to
fa063ab
Compare
|
@ienkovich, the error occurs if we import omniscidbe multiple times (>=2). Any thoughts on what happens when initializing omniscidbe that could affect the error? |
|
This pull request introduces 1 alert when merging fa063ab2a08b31efcb75a0984ca090251f1e6ac3 into d590de0 - view on LGTM.com new alerts:
|
I think the problem is not in double import but in import without dlopen flags. Look at how |
YarShev
force-pushed
the
dev/yigoshev-issue3774
branch
from
fa063ab
to
d421e23
Compare
Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com>
YarShev
force-pushed
the
dev/yigoshev-issue3774
branch
from
d421e23
to
2eae5be
Compare
YarShev
changed the title
vnlitvinov
requested changes
Dec 6, 2021
vnlitvinov
reviewed
Dec 7, 2021
YarShev
force-pushed
the
dev/yigoshev-issue3774
branch
from
8f7e538
to
2eae5be
Compare
Garra1980
reviewed
Dec 9, 2021
Garra1980
reviewed
Dec 9, 2021
|
This pull request introduces 2 alerts when merging e6eb6e3 into 61bf043 - view on LGTM.com new alerts:
|
YarShev
force-pushed
the
dev/yigoshev-issue3774
branch
from
965c045
to
774f7b5
Compare
|
This pull request introduces 2 alerts when merging 774f7b5 into 2cfabaf - view on LGTM.com new alerts:
|
Garra1980
added
the
Backport 🔙
|
@vnlitvinov, @devin-petersohn, @ienkovich, a reminder to review. |
vnlitvinov
requested changes
Dec 13, 2021
modin/experimental/core/execution/native/implementations/omnisci_on_native/utils.py
Outdated
Show resolved
Hide resolved
dchigarev
reviewed
Dec 13, 2021
modin/experimental/core/execution/native/implementations/omnisci_on_native/utils.py
Outdated
Show resolved
Hide resolved
YarShev
requested review from
ienkovich,
Garra1980,
vnlitvinov and
dchigarev
|
This pull request introduces 2 alerts when merging 62e2f3a into 0b7c642 - view on LGTM.com new alerts:
|
YarShev
changed the title
Garra1980
reviewed
Dec 14, 2021
| @@ -221,6 +220,11 @@ def gandiva_query(table, query): | |||
| expr = gen_table_expr(table, query) | |||
| if not can_be_condition(expr): | |||
| raise ValueError("Root operation should be a filter.") | |||
|
|
|||
| # We use this import here because of https://github.com/modin-project/modin/issues/3849, | |||
| # after the issue is fixed we should put the import at the top of this file | |||
There was a problem hiding this comment.
Probably add some xfail test?
There was a problem hiding this comment.
There is no any failing test now.
There was a problem hiding this comment.
I mean test that checks whether we ready to “put the import…”
There was a problem hiding this comment.
I see. We could add the following test:
class TestImportGandiva:
@pytest.mark.xfail(reason="core dump")
def test_import_gandiva(self):
import pyarrow.gandiva as gandivabut pytest is not able to handle this segfault. We just need to prioritize #3849 for 0.13 in order not to forget to fix it.
There was a problem hiding this comment.
We could implement it via launching something like subprocess.check_call([sys.executable, "-c", "import modin.experimental......PyDbEngine, pyarrow.gandiva"]) which would fail on non-zero exit code
There was a problem hiding this comment.
@dchigarev will tackle to this.
This was referenced Dec 14, 2021
devin-petersohn
removed
the
Backport 🔙
devin-petersohn
pushed a commit
that referenced
this pull request
Dec 16, 2021
dorisjlee
pushed a commit
to dorisjlee/modin
that referenced
this pull request
Jan 13, 2022
…odin-project#3783) * always import omnisci with correct dlopen flags * add a section on the issue in troubleshooting * worked around incompatibility of OmniSci and pyarrow.gandiva Signed-off-by: Igoshev, Yaroslav <yaroslav.igoshev@intel.com> Signed-off-by: Doris Lee <dorisjunglinlee@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Igoshev, Yaroslav yaroslav.igoshev@intel.com
What do these changes do?
There is an interesting thing with omnisci import. We have to import it with dlopen flags. Wherein, if we perform this, we have to import omnisci after Modin itself is imported, i.e. the following code snippet won't work:
The error is apparently arisen since there are other libraries which use LLVM and may register an option in the same address space.
This PR adds using subprocess to check whether omnisci is installed in order to return corresponding value from
Engine.get_default().flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/developer/architecture.rstis up-to-date