Restructure Html2Parquet with its own dpk_ namespace #809

touma-I · 2024-11-17T17:57:44Z

Why are these changes needed?

This is a first of a series of restructuring changes that are done to have each transform built as its own module (e.g. dpk_html2parquet) with a ray submodule (dpk_html2parquet.ray ).

Removed python and ray folders and keep Dockerfile.python and Dockerfile.ray
remove pyproject.toml
move python code under dpk_html2parquet
move ray code under dpk_html2parquet/ray
change import statement to include module name
replace recursive Makefile and use targets from .make.cicd.targets
adapt kfp_ray/Makefile and other make target

Related issue number (if any).

#774

Signed-off-by: Maroun Touma <[email protected]>

…o longer valid as it is based on folder name Signed-off-by: Maroun Touma <[email protected]>

Signed-off-by: Maroun Touma <[email protected]>

.make.defaults

transforms/.make.cicd.targets

transforms/language/html2parquet/dpk_html2parquet/transform.py

transforms/.make.cicd.targets

daw3rd · 2024-11-18T14:52:23Z

transforms/.make.cicd.targets

+build:: image
+
+publish:
+	@if [ -e Dockerfile.python ]; then \


maybe split this into publish-python/ray/spark.

@daw3rd When is publish being used. Do you know ?

daw3rd

This transform needs a README.md

.make.defaults

transforms/language/html2parquet/kfp_ray/Makefile

transforms/language/html2parquet/kfp_ray/html2parquet_wf.py

transforms/language/html2parquet/kfp_ray/pipeline_definitions.yaml

transforms/language/html2parquet/dpk_html2parquet/transform.py

daw3rd · 2024-11-18T15:43:14Z

transforms/language/html2parquet/dpk_html2parquet/transform.py

@@ -236,3 +236,35 @@ def apply_input_params(self, args: Namespace) -> bool:
        self.params = self.params | captured
        logger.info(f"html2parquet parameters are : {self.params}")
        return True
+
+
+class Html2Parquet(Html2ParquetTransform):


This class needs some documentation here and in a README.

BIG TIME :-). working on it. Thanks

transforms/language/html2parquet/Makefile

daw3rd · 2024-11-18T15:47:00Z

transforms/language/html2parquet/dpk_html2parquet/ray/transform.py

-    Html2ParquetTransform,
-    Html2ParquetTransformConfiguration,
-)
+from dpk_html2parquet.transform import Html2ParquetTransform, Html2ParquetTransformConfiguration


You need to run pre-commit.

@daw3rd can you elaborate ? what is pre-commit ?

daw3rd · 2024-11-18T15:48:02Z

transforms/language/html2parquet/python/README.md

This file can form the basis of a new readme in ../

Signed-off-by: Maroun Touma <[email protected]>

roytman

If we are doing so massive refactoring, should we combine test and test-data into one dir, e.g.

test
- data
  - input
  - expected

roytman · 2024-11-18T20:08:33Z

if each transformer builds its own module, should we add init.py files and create a unified namespace?
For example: instead
from dpk_html2parquet.transform import Html2ParquetTransformConfiguration
use
from dpk_html2parquet import Html2ParquetTransformConfiguration

the same for the ray runtime.

touma-I · 2024-11-18T21:54:23Z

If we are doing so massive refactoring, should we combine test and test-data into one dir, e.g.

test

data

input

expected

@roytman Why not leave it to the transform owner developer to decide if they want to nest the test-data under test. All we care about that we have a test folder for running the pytest. no ? where the developer puts their data is up to them. no ?

Signed-off-by: Maroun Touma <[email protected]>

touma-I · 2024-11-18T23:49:17Z

if each transformer builds its own module, should we add init.py files and create a unified namespace? For example: instead from dpk_html2parquet.transform import Html2ParquetTransformConfiguration use from dpk_html2parquet import Html2ParquetTransformConfiguration

the same for the ray runtime.

How did I miss that? Done. Thanks @roytman

Signed-off-by: Maroun Touma <[email protected]>

roytman · 2024-11-20T08:22:12Z

transforms/.make.cicd.targets

-TRANSFORM_RAY_RUNTIME_SRC_FILE=-m dpk_$(TRANSFORM_NAME).ray.transform
-TRANSFORM_PYTHON_RUNTIME_SRC_FILE=-m dpk_$(TRANSFORM_NAME).spark.transform
+## Default setting for TRANSFORM_RUNTIME entry point: 
+# python -m dpk_html2parquet.ray.transform --help


do we really need the dpk prefix for each transformer? If you want to prevent possible name conflicts, what about dpk namespace? e.g. dpk.html2parquet.ray.transform

@roytman : @dolfim-ibm made a passionate argument for this. I am good either way. @dolfim-ibm @daw3rd Can yo guys weigh in ? thanks

my argument was about having something, i.e. either prefix or namespace. I think it is very important to have a library which doesn't provide lots of global names which might overlap with others.
it also brings a bit of "branding" and makes it clearer to users that all these transforms come from the same place, i.e. data-prep-kit.

so why not dpk.html2parquet.ray.transform ?

It's just a suggestion; I'm OK with any decision.

sungeunan-ibm · 2024-11-21T00:00:44Z

transforms/language/html2parquet/notebooks/html2parquet.ipynb

+   ],
+   "source": [
+    "from dpk_html2parquet.transform_python import Html2ParquetRuntime\n",
+    "x=Html2ParquetRuntime(input_folder= \"input\", \n",


"input/ai-alliance-index.html"

sungeunan-ibm · 2024-11-21T00:01:24Z

transforms/language/html2parquet/notebooks/html2parquet.ipynb

+   "source": [
+    "from dpk_html2parquet.transform_python import Html2ParquetRuntime\n",
+    "x=Html2ParquetRuntime(input_folder= \"input\", \n",
+    "               output_folder= \"output\", \n",


Do we need an output folder for this to work?

sungeunan-ibm · 2024-11-21T00:02:10Z

transforms/language/html2parquet/notebooks/html2parquet.ipynb

+    "from dpk_html2parquet.transform_python import Html2ParquetRuntime\n",
+    "x=Html2ParquetRuntime(input_folder= \"input\", \n",
+    "               output_folder= \"output\", \n",
+    "               data_files_to_use=['.html']).transform()"


I know input file is just one html file but ['.zip', '.html'] in case user wants to test html zip in the notebook.

or add a comment something like: '.zip' in case your input file is zip of html files.

sungeunan-ibm

I've reviewed all the changes, and they all make sense to me. I left some comments. Thank you for the significant effort you put into restructuring the code! @touma-I

touma-I added 13 commits November 13, 2024 17:03

html2parquet example

5ca102f

Signed-off-by: Maroun Touma <[email protected]>

fix typo with package name

ee1b3c5

Signed-off-by: Maroun Touma <[email protected]>

remove output folder from git

244a7b3

Merge branch 'dev' into html2parquet-simplify

3b8576d

first draft make test-src working

402cfb9

Signed-off-by: Maroun Touma <[email protected]>

fix docker build for all 3 runtime

078179a

Signed-off-by: Maroun Touma <[email protected]>

fix publish-image

a222cb3

Signed-off-by: Maroun Touma <[email protected]>

use module name to invoke entry point

d3df908

Signed-off-by: Maroun Touma <[email protected]>

added kind-load-image required for workflow test

00c2d70

Signed-off-by: Maroun Touma <[email protected]>

fix setting for transform runtime being used. The global setting is n…

c128893

…o longer valid as it is based on folder name Signed-off-by: Maroun Touma <[email protected]>

override DOCKER_REMOTE_IMAGE defined in .make.defaults

b44fff1

Signed-off-by: Maroun Touma <[email protected]>

chasing after bug with DOCKER_REMOTE_IMAGE value

6631851

Signed-off-by: Maroun Touma <[email protected]>

chasing after DOCKER_REMOTE_IMAGE

1c58a5c

Signed-off-by: Maroun Touma <[email protected]>

touma-I requested review from roytman, revit13, daw3rd and shivdeep-singh-ibm November 18, 2024 01:47

bug fixes

981930c

Signed-off-by: Maroun Touma <[email protected]>

daw3rd requested changes Nov 18, 2024

View reviewed changes

touma-I added 2 commits November 18, 2024 11:40

odify notebook with simplified API

3481667

Signed-off-by: Maroun Touma <[email protected]>

keep for comparaison

890a9da

Signed-off-by: Maroun Touma <[email protected]>

touma-I requested a review from sungeunan-ibm November 18, 2024 17:19

fix typo

e99e0bc

Signed-off-by: Maroun Touma <[email protected]>

roytman reviewed Nov 18, 2024

View reviewed changes

added __init__

590f5dd

Signed-off-by: Maroun Touma <[email protected]>

touma-I added 2 commits November 18, 2024 18:53

no longer needed

777fb9d

Signed-off-by: Maroun Touma <[email protected]>

Addd test-image-python/ray/spark

d7dfa3e

Signed-off-by: Maroun Touma <[email protected]>

touma-I marked this pull request as ready for review November 19, 2024 01:44

touma-I marked this pull request as draft November 19, 2024 12:46

shahrokhDaijavad mentioned this pull request Nov 19, 2024

Html2Parquet Updated README and Added Sample Notebook #815

Merged

revit13 approved these changes Nov 20, 2024

View reviewed changes

roytman reviewed Nov 20, 2024

View reviewed changes

sungeunan-ibm reviewed Nov 21, 2024

View reviewed changes

sungeunan-ibm approved these changes Nov 21, 2024

View reviewed changes

roytman approved these changes Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure Html2Parquet with its own dpk_ namespace #809

Restructure Html2Parquet with its own dpk_ namespace #809

touma-I commented Nov 17, 2024 •

edited

Loading

daw3rd Nov 18, 2024

touma-I Nov 18, 2024

daw3rd left a comment

daw3rd Nov 18, 2024

touma-I Nov 18, 2024

daw3rd Nov 18, 2024

touma-I Nov 18, 2024

daw3rd Nov 18, 2024

roytman left a comment

roytman commented Nov 18, 2024

touma-I commented Nov 18, 2024 •

edited

Loading

touma-I commented Nov 18, 2024

roytman Nov 20, 2024

touma-I Nov 20, 2024

dolfim-ibm Nov 20, 2024

roytman Nov 20, 2024

roytman Nov 20, 2024

sungeunan-ibm Nov 21, 2024

sungeunan-ibm Nov 21, 2024

sungeunan-ibm Nov 21, 2024

sungeunan-ibm Nov 21, 2024

sungeunan-ibm left a comment

Restructure Html2Parquet with its own dpk_ namespace #809

Are you sure you want to change the base?

Restructure Html2Parquet with its own dpk_ namespace #809

Conversation

touma-I commented Nov 17, 2024 • edited Loading

Why are these changes needed?

Related issue number (if any).

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daw3rd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roytman left a comment

Choose a reason for hiding this comment

roytman commented Nov 18, 2024

touma-I commented Nov 18, 2024 • edited Loading

touma-I commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sungeunan-ibm left a comment

Choose a reason for hiding this comment

touma-I commented Nov 17, 2024 •

edited

Loading

touma-I commented Nov 18, 2024 •

edited

Loading