FEAT-#4946: Replace OmniSci with HDK #4947

AndreyPavlenko · 2022-09-08T21:42:30Z

What do these changes do?

OmniSci has been replaced in the code, paths and documentation.
Simple renaming is not sufficient for the documentation. An
additional work is required to review and rewrite the documentation.

commit message follows format outlined here
passes flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
passes black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
signed commit with git commit -s
Resolves REFACTOR: Replace OmniSci with HDK #4946
tests added and passing
module layout described at docs/development/architecture.rst is up-to-date
added (Issue Number: PR title (PR Number)) and github username to release notes for next major release

codecov · 2022-09-08T21:52:02Z

Codecov Report

Merging #4947 (e89c521) into master (d6d503a) will increase coverage by 4.69%.
The diff coverage is 88.70%.

@@            Coverage Diff             @@
##           master    #4947      +/-   ##
==========================================
+ Coverage   84.91%   89.60%   +4.69%     
==========================================
  Files         267      267              
  Lines       19744    20044     +300     
==========================================
+ Hits        16766    17961    +1195     
+ Misses       2978     2083     -895

Impacted Files	Coverage Δ
modin/experimental/cloud/hdk.py	`0.00% <0.00%> (ø)`
...e/implementations/hdk_on_native/calcite_builder.py	`96.36% <ø> (ø)`
...mplementations/hdk_on_native/calcite_serializer.py	`98.70% <ø> (ø)`
...native/implementations/hdk_on_native/df_algebra.py	`76.88% <ø> (ø)`
...ution/native/implementations/hdk_on_native/expr.py	`85.63% <ø> (ø)`
...imental/core/storage_formats/hdk/query_compiler.py	`94.49% <ø> (ø)`
modin/experimental/cloud/cluster.py	`79.20% <50.00%> (+20.79%)`	⬆️
modin/pandas/__init__.py	`71.26% <53.84%> (+4.59%)`	⬆️
.../native/implementations/hdk_on_native/db_worker.py	`87.50% <66.66%> (ø)`
modin/utils.py	`93.66% <66.66%> (+14.02%)`	⬆️
... and 66 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lgtm-com · 2022-09-08T22:15:03Z

This pull request fixes 2 alerts when merging 6f77cf2eb6728beafcfdb02a0b8d95fcd2f56da3 into a6f47c8 - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2022-09-08T23:42:51Z

This pull request fixes 2 alerts when merging a14162a4c22bc4fb13deb5b81fdc7820f052af51 into a6f47c8 - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2022-09-09T01:01:16Z

This pull request fixes 2 alerts when merging 4308319125347108f874b7ff2547c028f7110957 into a6f47c8 - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2022-09-09T20:53:39Z

This pull request fixes 2 alerts when merging bd2c0ebf876b274163d2b8ab45b645fed9a95296 into b09dd42 - view on LGTM.com

fixed alerts:

2 for Unused import

docs/getting_started/installation.rst

Garra1980 · 2022-09-12T16:49:47Z

images with OmniscServer chould be changed - luckily we got rid of it

modin/experimental/core/execution/native/implementations/hdk_on_native/db_worker.py

modin/pandas/__init__.py

lgtm-com · 2022-09-13T20:23:49Z

This pull request fixes 2 alerts when merging 04b3947a172c6cb94472d8e4387da23286126c78 into af6f827 - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2022-09-14T12:19:19Z

This pull request fixes 2 alerts when merging fc9e0241a272048fdf34e89c37686a0c92ffa45e into af6f827 - view on LGTM.com

fixed alerts:

2 for Unused import

CODEOWNERS

docs/development/using_hdk.rst

docs/flow/modin/experimental/core/execution/native/implementations/hdk_on_native/hdk_worker.rst

docs/flow/modin/experimental/core/execution/native/implementations/hdk_on_native/index.rst

modin/config/envvars.py

modin/experimental/core/execution/native/__init__.py

modin/experimental/core/execution/native/implementations/hdk_on_native/base_worker.py

modin/pandas/__init__.py

scripts/doc_checker.py

YarShev · 2022-09-19T09:13:51Z

@ienkovich, could you also read through the changes (both code and docs)?

ienkovich

docs/img/hdk/hdk_import.svg still has OmniSciServer. Now It's probably HDK Library or HDK Engine or something like that.

Same thing in hdk_query_flow.svg

examples/docker/modin-hdk/census-hdk.py

modin/utils.py

scripts/doc_checker.py

docs/flow/modin/experimental/core/execution/native/implementations/hdk_on_native/index.rst

lgtm-com · 2022-09-19T20:06:59Z

This pull request fixes 2 alerts when merging e3fac0d595459f02903c8ccc694362040abcc79d into 7c260ad - view on LGTM.com

fixed alerts:

2 for Unused import

YarShev · 2022-09-19T21:49:35Z

@AndreyPavlenko, please ping me when comments are addressed.

AndreyPavlenko · 2022-09-20T05:29:20Z

@AndreyPavlenko, please ping me when comments are addressed.

Do you mean doc_checker.py? Comments added.

YarShev · 2022-09-20T07:28:41Z

No, I mean when all comments are addressed and the PR is ready for review again.

AndreyPavlenko · 2022-09-20T07:32:12Z

No, I mean when all comments are addressed and the PR is ready for review again.

Yes, I've rebased the commit accordingly.

vnlitvinov · 2022-09-20T08:34:22Z

@AndreyPavlenko please note that "refactoring" is changing some code implementation without changing any outside behaviour. What you've done here should be labelled FEAT as it's a feature change.

AndreyPavlenko · 2022-09-20T09:24:41Z

Aside: It would be better to address comments in separate commits to make it easier for reviewers inspect changes.

Should I do multiple commits (one per comment) and then squash the commits before the merge?

YarShev · 2022-09-20T09:29:46Z

Aside: It would be better to address comments in separate commits to make it easier for reviewers inspect changes.

Should I do multiple commits (one per comment) and then squash the commits before the merge?

You can do as much commits as you want. The reviewer who is going to merge the changes will squash the commits. We require only the first commit to follow the contributing guideline. Btw, you can apply all suggestions in one commit as batch, GitHub allows to do that.

lgtm-com · 2022-09-20T10:43:01Z

This pull request fixes 2 alerts when merging 939693db4f4e10a47d609171eafc979e491e1eab into c10f0f9 - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2022-09-20T11:30:42Z

This pull request fixes 2 alerts when merging 2a9607b0a477e9c2dc53d493bcbd086a5a77cdfc into c10f0f9 - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2022-09-20T17:37:27Z

This pull request fixes 2 alerts when merging b5e7dec4d061f63568cf38ac8c1c9c9f66effee0 into 45879a6 - view on LGTM.com

fixed alerts:

2 for Unused import

lgtm-com · 2022-09-20T19:46:24Z

This pull request fixes 2 alerts when merging a896407b2d072a5ed3bdeae0d2185218f28d1399 into 45879a6 - view on LGTM.com

fixed alerts:

2 for Unused import

YarShev · 2022-09-21T09:01:16Z

@AndreyPavlenko, please resolve conflicts.

OmniSci has been replaced in the code, paths and documentation. Simple renaming is not sufficient for the documentation. An additional work is required to review and rewrite the documentation. Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>

Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>

Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>

Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>

AndreyPavlenko · 2022-09-21T09:10:06Z

@AndreyPavlenko, please resolve conflicts.

Done.

lgtm-com · 2022-09-21T09:58:01Z

This pull request fixes 2 alerts when merging 1c4d19e into d6d503a - view on LGTM.com

fixed alerts:

2 for Unused import

docs/getting_started/installation.rst

Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>

lgtm-com · 2022-09-21T15:20:16Z

This pull request fixes 2 alerts when merging e6862829d4aac6e95c4fa65073872f6b597e057b into d6d503a - view on LGTM.com

fixed alerts:

2 for Unused import

Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>

lgtm-com · 2022-09-21T16:53:37Z

This pull request fixes 2 alerts when merging e89c521 into d6d503a - view on LGTM.com

fixed alerts:

2 for Unused import

AndreyPavlenko force-pushed the refactor-hdk branch from 6f77cf2 to a14162a Compare September 8, 2022 23:08

AndreyPavlenko force-pushed the refactor-hdk branch from a14162a to 4308319 Compare September 9, 2022 00:24

AndreyPavlenko force-pushed the refactor-hdk branch 2 times, most recently from 57857ec to bd2c0eb Compare September 9, 2022 20:24

Garra1980 reviewed Sep 12, 2022

View reviewed changes

docs/getting_started/installation.rst Outdated Show resolved Hide resolved

Garra1980 reviewed Sep 12, 2022

View reviewed changes

modin/experimental/core/execution/native/implementations/hdk_on_native/db_worker.py Show resolved Hide resolved

Garra1980 reviewed Sep 12, 2022

View reviewed changes

modin/pandas/__init__.py Outdated Show resolved Hide resolved

AndreyPavlenko force-pushed the refactor-hdk branch from bd2c0eb to 04b3947 Compare September 13, 2022 19:47

AndreyPavlenko force-pushed the refactor-hdk branch from 04b3947 to fc9e024 Compare September 14, 2022 11:47

AndreyPavlenko marked this pull request as ready for review September 14, 2022 18:46

AndreyPavlenko requested review from a team as code owners September 14, 2022 18:46

YarShev reviewed Sep 15, 2022

View reviewed changes

ienkovich reviewed Sep 19, 2022

View reviewed changes

examples/docker/modin-hdk/census-hdk.py Show resolved Hide resolved

modin/utils.py Show resolved Hide resolved

scripts/doc_checker.py Outdated Show resolved Hide resolved

docs/flow/modin/experimental/core/execution/native/implementations/hdk_on_native/index.rst Show resolved Hide resolved

AndreyPavlenko force-pushed the refactor-hdk branch 2 times, most recently from 453d606 to e3fac0d Compare September 19, 2022 19:35

AndreyPavlenko force-pushed the refactor-hdk branch from 939693d to 2a9607b Compare September 20, 2022 10:53

AndreyPavlenko changed the title ~~REFACTOR-#4946: Replace OmniSci with HDK~~ FEAT-#4946: Replace OmniSci with HDK Sep 20, 2022

AndreyPavlenko force-pushed the refactor-hdk branch from ca3e408 to a896407 Compare September 20, 2022 19:09

YarShev previously approved these changes Sep 21, 2022

View reviewed changes

AndreyPavlenko and others added 5 commits September 21, 2022 11:08

Apply suggestions from code review

09ec3aa

Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>

Apply suggestions from code review

2c717ba

Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>

Use fixed phdk version - 0.1

147db58

Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>

Apply suggestions from code review

1c4d19e

Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>

AndreyPavlenko dismissed YarShev’s stale review via 1c4d19e September 21, 2022 09:09

AndreyPavlenko force-pushed the refactor-hdk branch from a896407 to 1c4d19e Compare September 21, 2022 09:09

YarShev reviewed Sep 21, 2022

View reviewed changes

docs/getting_started/installation.rst Outdated Show resolved Hide resolved

docs/getting_started/installation.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

7e4b5ea

Co-authored-by: Iaroslav Igoshev <Poolliver868@mail.ru>

Added out-of-date note to docs

e89c521

Signed-off-by: Andrey Pavlenko <andrey.a.pavlenko@gmail.com>

AndreyPavlenko force-pushed the refactor-hdk branch from e686282 to e89c521 Compare September 21, 2022 15:56

YarShev approved these changes Sep 21, 2022

View reviewed changes

YarShev merged commit e5b1888 into modin-project:master Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT-#4946: Replace OmniSci with HDK #4947

FEAT-#4946: Replace OmniSci with HDK #4947

AndreyPavlenko commented Sep 8, 2022 •

edited

Loading

codecov bot commented Sep 8, 2022 •

edited

Loading

lgtm-com bot commented Sep 8, 2022

lgtm-com bot commented Sep 8, 2022

lgtm-com bot commented Sep 9, 2022

lgtm-com bot commented Sep 9, 2022

Garra1980 commented Sep 12, 2022

lgtm-com bot commented Sep 13, 2022

lgtm-com bot commented Sep 14, 2022

YarShev commented Sep 19, 2022

ienkovich left a comment

lgtm-com bot commented Sep 19, 2022

YarShev commented Sep 19, 2022

AndreyPavlenko commented Sep 20, 2022

YarShev commented Sep 20, 2022

AndreyPavlenko commented Sep 20, 2022

vnlitvinov commented Sep 20, 2022

AndreyPavlenko commented Sep 20, 2022

YarShev commented Sep 20, 2022 •

edited

Loading

lgtm-com bot commented Sep 20, 2022

lgtm-com bot commented Sep 20, 2022

lgtm-com bot commented Sep 20, 2022

lgtm-com bot commented Sep 20, 2022

YarShev commented Sep 21, 2022

AndreyPavlenko commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

FEAT-#4946: Replace OmniSci with HDK #4947

FEAT-#4946: Replace OmniSci with HDK #4947

Conversation

AndreyPavlenko commented Sep 8, 2022 • edited Loading

What do these changes do?

codecov bot commented Sep 8, 2022 • edited Loading

Codecov Report

lgtm-com bot commented Sep 8, 2022

lgtm-com bot commented Sep 8, 2022

lgtm-com bot commented Sep 9, 2022

lgtm-com bot commented Sep 9, 2022

Garra1980 commented Sep 12, 2022

lgtm-com bot commented Sep 13, 2022

lgtm-com bot commented Sep 14, 2022

YarShev commented Sep 19, 2022

ienkovich left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Sep 19, 2022

YarShev commented Sep 19, 2022

AndreyPavlenko commented Sep 20, 2022

YarShev commented Sep 20, 2022

AndreyPavlenko commented Sep 20, 2022

vnlitvinov commented Sep 20, 2022

AndreyPavlenko commented Sep 20, 2022

YarShev commented Sep 20, 2022 • edited Loading

lgtm-com bot commented Sep 20, 2022

lgtm-com bot commented Sep 20, 2022

lgtm-com bot commented Sep 20, 2022

lgtm-com bot commented Sep 20, 2022

YarShev commented Sep 21, 2022

AndreyPavlenko commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

lgtm-com bot commented Sep 21, 2022

AndreyPavlenko commented Sep 8, 2022 •

edited

Loading

codecov bot commented Sep 8, 2022 •

edited

Loading

YarShev commented Sep 20, 2022 •

edited

Loading