add license check transform #257

shivdeep-singh-ibm · 2024-06-10T15:22:30Z

Why are these changes needed?

Add license check filter

Related issue number (if any).

transforms/code/license_check/python/src/dpk_license_check_python/internal/__init__.py

transforms/code/license_check/python/src/license_check_local_pure.py

transforms/code/license_check/python/src/license_check_local_python.py

transforms/code/license_check/python/src/dpk_license_check_python/transform_config.py

.make.versions

transforms/code/license_check/python/Dockerfile

transforms/code/license_check/python/src/license_check_local_python.py

transforms/code/license_check/python/src/license_check_transform.py

transforms/code/license_check/python/test-data/expected/metadata.json

.make.versions

transforms/code/license_check/ray/Dockerfile

Bytes-Explorer · 2024-06-17T11:00:57Z

@shivdeep-singh-ibm This module (folder) should be called license filtering

revit13 · 2024-06-19T07:18:31Z

@shivdeep-singh-ibm it seems that the workflow- rules in transforms/code/license_check/Makefile needs to be changed (they refer to kfp_ray/v1 directory which does not exist). This causes the CI fail: Please see the Makefile in other code transforms to fix that.

make[4]: Leaving directory '/home/runner/work/data-prep-kit/data-prep-kit/transforms/code/ingest_2_parquet'
Using recursive workflow-build rule in license_check/
make[4]: Entering directory '/home/runner/work/data-prep-kit/data-prep-kit/transforms/code/license_check'
make -C kfp_ray/v1 workflow-build
make[5]: *** kfp_ray/v1: No such file or directory.  Stop.

transforms/code/license_check/kfp_ray/license_check_wf.py

transforms/code/license_check/python/pyproject.toml

transforms/code/license_check/ray/pyproject.toml

transforms/code/license_check/python/src/license_check_transform.py

transforms/code/license_check/python/Makefile

transforms/code/license_check/python/README.md

transforms/code/license_check/ray/src/license_check_local_ray.py

transforms/code/license_check/python/README.md

transforms/code/license_check/ray/test/test_license_check_ray.py

Param-S

minor doc/readme comment. otherwise changes good to me.

Param-S · 2024-06-27T18:03:15Z

transforms/code/license_check/README.md

@@ -0,0 +1,13 @@
+# License Check 
+
+The License Check transform checks if the '`license` of input data is in approved/denied list as 


This sentence looks incomplete. The transformer verifies and marks the sample accordingly in the output right? should we convey that in this description.

daw3rd · 2024-06-28T00:22:18Z

@shivdeep-singh-ibm suggest you merge dev into this branch given some recent issues with versioning. See #355

transforms/code/license_check/python/src/license_check_transform.py

transforms/code/license_check/python/README.md

transforms/code/license_check/python/src/license_check_transform.py

daw3rd · 2024-07-01T15:27:14Z

transforms/code/license_check/python/README.md

+  `--lc_license_column_name` - set the name of the column holds license to process
+  `--lc_allow_no_license` - allow entries with no associated license (default: false)
+  `--lc_licenses_file` - S3 or local path to allowed/denied licenses JSON file
+  `--lc_deny_licenses` - allow all licences except those in licenses_file (default: false)


Because you have your own DataAccessFactory, aren't there some lc_data params that would allow reading the license file from S3, for example.

A hidden feature, to see the --help text for a given transform

make .transforms.test-image-help

This should show the -lc_data_* options

no additional data options used.

Unless I'm missing something, the code below from line 134 of this file will add the --lc_data_* parameters (specifically the last line below).

# Create the DataAccessFactor to use CLI args self.daf = DataAccessFactory(CLI_PREFIX, False) # Add the DataAccessFactory parameters to the transform's configuration parameters. self.daf.add_input_params(parser)

transforms/code/license_check/python/pyproject.toml

transforms/code/license_check/ray/pyproject.toml

transforms/code/license_check/python/README.md

daw3rd

In addtion to my specific comments. a more global comment is to rename this to license_select which then matches proglang_select naming.

transforms/code/license_check/ray/Makefile

daw3rd · 2024-07-15T15:42:49Z

transforms/code/license_check/README.md

@@ -0,0 +1,11 @@
+# License Check 
+
+The License Check transform checks if the `license` of input data is in approved/denied list. It is implemented as per the set of [transform project conventions](../../README.md#transform-project-conventions) the following runtimes are available:


Can you be more specific and say it selects only the specified licenses and filters out the other licenses..

daw3rd · 2024-07-15T15:44:33Z

transforms/code/license_check/kfp_ray/license_check_wf.py

+task_image = "quay.io/dataprep1/data-prep-kit/license_check-ray:0.4.0.dev6"
+
+# components
+base_kfp_image = "quay.io/dataprep1/data-prep-kit/kfp-data-processing:0.2.0.dev6"


Think you need to run make set-versions as these are old. Probably should be 0.2.1.dev0 everywhere

daw3rd · 2024-07-15T15:45:27Z

transforms/code/license_check/python/Makefile

+
+# distribution versions is the same as image version.
+set-versions:
+	$(MAKE) TRANSFORM_PYTHON_VERSION=${LICENSE_CHECK_PYTHON_VERSION} .transforms.set-versions 


This now back to requiring TOML_VERSION. For example, from noop

$(MAKE) TRANSFORM_PYTHON_VERSION=$(NOOP_PYTHON_VERSION) TOML_VERSION=$(NOOP_PYTHON_VERSION) .transforms.set-versions ```

daw3rd · 2024-07-15T15:47:43Z

transforms/code/license_check/python/Makefile

+set-versions:
+	$(MAKE) TRANSFORM_PYTHON_VERSION=${LICENSE_CHECK_PYTHON_VERSION} .transforms.set-versions 
+
+build-dist:: set-versions .defaults.build-dist 


You can remove set-versions here. The convention now is that this generally only to be called from the top of the repo outside of the makefiles (unless you get out of sync, like you are in the the _wf.py file and maybe others)

daw3rd · 2024-07-15T15:49:21Z

transforms/code/license_check/python/README.md

+
+## Summary 
+
+This filter scans the license column of an input dataset and appends the `license_status` column to the dataset.


Can you be more specific and say it selects only the specified licenses and filters out the other licenses. (same comment as on the top level readme)

daw3rd · 2024-07-15T15:50:49Z

transforms/code/license_check/python/README.md

+Then
+
+ls output
+To see results of the transform.


You should add the same last paragraph that all transforms now have...https://github.com/IBM/data-prep-kit/tree/dev/transforms/universal/noop/python#transforming-data-using-the-transform-image

daw3rd · 2024-07-15T15:55:29Z

transforms/code/license_check/ray/Makefile

+set-versions:
+	$(MAKE) TRANSFORM_PYTHON_VERSION=${LICENSE_CHECK_RAY_VERSION} .transforms.set-versions
+
+build-dist:: set-versions .defaults.build-dist 


Remove set-versions here

daw3rd · 2024-07-15T19:28:36Z

Also, not sure how far behind dev this is. It's probably a good idea to merge with dev

daw3rd · 2024-07-30T14:08:58Z

transforms/code/license_select/kfp_ray/license_select_wf.py

+EXEC_SCRIPT_NAME: str = "license_select_transform_ray.py"
+PREFIX: str = ""
+
+task_image = "quay.io/dataprep1/data-prep-kit/license_select-ray:0.4.0.dev6"


we are now using latest tag on images

daw3rd · 2024-07-30T14:12:56Z

transforms/code/license_select/python/Dockerfile

+RUN pip install --no-cache-dir -e .
+
+# Added to enable running test 
+RUN cp src/license_select_transform_python.py . 


I think this can/should be removed.

daw3rd · 2024-07-30T14:13:25Z

transforms/code/license_select/python/Dockerfile

+
+# Set environment
+ENV PYTHONPATH /home/dpk
+ENV PATH $PATH:/home/dpk/.local/bin/


Curious why is this added?

daw3rd · 2024-07-30T14:14:17Z

transforms/code/license_select/python/Makefile

+
+TRANSFORM_NAME=license_select
+# $(REPOROOT)/.make.versions file contains the versions
+DOCKER_IMAGE_VERSION=${LICENSE_SELECT_PYTHON_VERSION}


DOCKER_IMAGE_VERSION can be removed.

@shivdeep-singh-ibm can you please check this input & address it

daw3rd · 2024-07-30T14:24:37Z

transforms/code/license_select/ray/Makefile

+
+TRANSFORM_NAME=license_select
+# $(REPOROOT)/.make.versions file contains the versions
+DOCKER_IMAGE_VERSION=${LICENSE_SELECT_RAY_VERSION}


DOCKER_IMAGE_VERSION can be removed.

daw3rd · 2024-07-30T14:25:02Z

transforms/code/license_select/ray/Dockerfile

+# copy test
+COPY test/ test/
+COPY test-data/ test-data/
+ENV PATH $PATH:/home/dpk/.local/bin/


Curious why you are setting PATH?

daw3rd · 2024-07-30T14:27:08Z

transforms/code/license_select/ray/Dockerfile

@@ -0,0 +1,39 @@
+FROM docker.io/rayproject/ray:2.9.3-py310


The first lines of this file need to be updated. From code2parquet, for example

ARG BASE_IMAGE=docker.io/rayproject/ray:2.24.0-py310 FROM ${BASE_IMAGE} RUN pip install --upgrade --no-cache-dir pip # install pytest RUN pip install --no-cache-dir pytest

daw3rd · 2024-07-30T14:28:05Z

transforms/code/license_select/ray/Dockerfile

+RUN cd python-transform && pip install --no-cache-dir -e .
+
+COPY --chown=ray:users python-transform lc_python
+RUN cd lc_python && pip install .


Isn't this is a duplication of lines 12 and 13 above (the previous 2 statements)?

daw3rd · 2024-07-30T18:53:10Z

transforms/code/license_select/kfp_ray/license_select_wf.py

+task_image = "quay.io/dataprep1/data-prep-kit/license_select-ray:0.4.0.dev6"
+
+# components
+base_kfp_image = "quay.io/dataprep1/data-prep-kit/kfp-data-processing:0.2.0.dev6"


and this should be 0.2.1.dev0. you might try running make set-versions from ..

oops this should also be latest

daw3rd · 2024-08-08T13:44:53Z

transforms/code/license_select/python/README.md

+    }
+}
+```
+license_column_name - The name of the column with licenses.


this should be a list of bullets. as is, it is wrapping into a single paragraph.

Param-S

lgtm

Signed-off-by: Shivdeep Singh <[email protected]>

shivdeep-singh-ibm requested review from daw3rd and Param-S June 10, 2024 15:22

daw3rd added the enhancement New feature or request label Jun 10, 2024

shivdeep-singh-ibm force-pushed the add_license_check_filter branch 2 times, most recently from 378a170 to b539226 Compare June 11, 2024 18:02

daw3rd requested changes Jun 13, 2024

View reviewed changes

shivdeep-singh-ibm force-pushed the add_license_check_filter branch from 3b48683 to 2066cb8 Compare June 14, 2024 10:01

Param-S requested changes Jun 14, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

shivdeep-singh-ibm force-pushed the add_license_check_filter branch from 2758803 to 7b7597c Compare June 20, 2024 05:21

revit13 reviewed Jun 20, 2024

View reviewed changes

transforms/code/license_check/kfp_ray/license_check_wf.py Outdated Show resolved Hide resolved

shivdeep-singh-ibm force-pushed the add_license_check_filter branch 3 times, most recently from 41bcc4f to d3d6b41 Compare June 26, 2024 07:53

shivdeep-singh-ibm requested review from daw3rd and Param-S June 26, 2024 14:22

daw3rd requested changes Jun 26, 2024

View reviewed changes

daw3rd requested changes Jun 27, 2024

View reviewed changes

transforms/code/license_check/python/README.md Outdated Show resolved Hide resolved

transforms/code/license_check/python/README.md Outdated Show resolved Hide resolved

transforms/code/license_check/ray/test/test_license_check_ray.py Outdated Show resolved Hide resolved

Param-S reviewed Jun 27, 2024

View reviewed changes

shivdeep-singh-ibm force-pushed the add_license_check_filter branch from c39d8ea to 09221dd Compare June 27, 2024 18:52

shivdeep-singh-ibm force-pushed the add_license_check_filter branch from bdf4f98 to cff9a6d Compare June 28, 2024 09:34

daw3rd requested changes Jun 28, 2024

View reviewed changes

transforms/code/license_check/python/src/license_check_transform.py Outdated Show resolved Hide resolved

transforms/code/license_check/python/README.md Outdated Show resolved Hide resolved

daw3rd requested changes Jul 1, 2024

View reviewed changes

daw3rd requested changes Jul 2, 2024

View reviewed changes

transforms/code/license_check/python/README.md Outdated Show resolved Hide resolved

roytman mentioned this pull request Jul 9, 2024

Update KFP super pipeline for code #388

Open

shivdeep-singh-ibm force-pushed the add_license_check_filter branch 2 times, most recently from 3ffc512 to c43993e Compare July 15, 2024 10:34

shivdeep-singh-ibm force-pushed the add_license_check_filter branch from c43993e to 24121c3 Compare July 15, 2024 14:21

daw3rd requested changes Jul 15, 2024

View reviewed changes

shivdeep-singh-ibm force-pushed the add_license_check_filter branch 4 times, most recently from fed1461 to e4d6efa Compare July 26, 2024 04:19

daw3rd requested changes Jul 30, 2024

View reviewed changes

daw3rd requested changes Aug 8, 2024

View reviewed changes

roytman mentioned this pull request Aug 19, 2024

Add Licence transforms to Code Superpipeline. #400

Merged

shivdeep-singh-ibm force-pushed the add_license_check_filter branch 3 times, most recently from 1293669 to 0f2c5b6 Compare September 3, 2024 17:26

Param-S approved these changes Sep 4, 2024

View reviewed changes

daw3rd mentioned this pull request Sep 13, 2024

[Feature] Add license filtering for code modules #139

Closed

2 tasks

shivdeep-singh-ibm force-pushed the add_license_check_filter branch from d9357c8 to 9e4ac28 Compare September 30, 2024 08:18

Add license select filter

6cb888b

Signed-off-by: Shivdeep Singh <[email protected]>

shivdeep-singh-ibm force-pushed the add_license_check_filter branch from 9e4ac28 to 6cb888b Compare September 30, 2024 08:36

shivdeep-singh-ibm merged commit 7a8a949 into IBM:dev Sep 30, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add license check transform #257

add license check transform #257

shivdeep-singh-ibm commented Jun 10, 2024

Bytes-Explorer commented Jun 17, 2024

This comment was marked as resolved.

revit13 commented Jun 19, 2024

Param-S left a comment

Param-S Jun 27, 2024

daw3rd commented Jun 28, 2024

daw3rd Jul 1, 2024

daw3rd Jul 1, 2024

shivdeep-singh-ibm Jul 15, 2024

daw3rd Jul 15, 2024

daw3rd left a comment

daw3rd Jul 15, 2024

daw3rd Jul 15, 2024

daw3rd Jul 15, 2024

daw3rd Jul 15, 2024

daw3rd Jul 15, 2024

daw3rd Jul 15, 2024

daw3rd Jul 15, 2024

daw3rd commented Jul 15, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

Param-S Sep 3, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

daw3rd Jul 30, 2024

daw3rd Aug 8, 2024

shivdeep-singh-ibm Sep 4, 2024

Param-S left a comment

		@@ -0,0 +1,13 @@
		# License Check

		The License Check transform checks if the '`license` of input data is in approved/denied list as

		@@ -0,0 +1,11 @@
		# License Check

		The License Check transform checks if the `license` of input data is in approved/denied list. It is implemented as per the set of [transform project conventions](../../README.md#transform-project-conventions) the following runtimes are available:


		## Summary

		This filter scans the license column of an input dataset and appends the `license_status` column to the dataset.

add license check transform #257

add license check transform #257

Conversation

shivdeep-singh-ibm commented Jun 10, 2024

Why are these changes needed?

Related issue number (if any).

Bytes-Explorer commented Jun 17, 2024

This comment was marked as resolved.

revit13 commented Jun 19, 2024

Param-S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daw3rd commented Jun 28, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daw3rd commented Jul 15, 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

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

Param-S left a comment

Choose a reason for hiding this comment