adding hap transform #638

ian-cho · 2024-09-28T14:05:55Z

Why are these changes needed?

adding python implementation of hap transform under universal directory for DPK outer. Passed all tests when "make test". please review. Thanks.

.github/workflows/test-universal-hap.yml

transforms/universal/hap/python/src/hap_transform.py

transforms/universal/hap/python/pyproject.toml

transforms/universal/hap/python/requirements.txt

transforms/universal/hap/python/README.md

daw3rd · 2024-09-30T13:04:35Z

transforms/universal/hap/python/README.md

+
+* --model_name_or_path - specifies HAP model which should be compatable with HuggingFace's `AutoModelForSequenceClassification` 
+* --batch_size - modify it based on the infrastructure capacity.
+* --max_length - the maximum length for the tokenizer.


I think you are are missing the annotation_column and document_column configuraitions.

Also, can you change to the format for documenting configuration used here https://github.com/IBM/data-prep-kit/tree/dev/transforms/language/pdf2parquet/python?

daw3rd · 2024-09-30T13:06:25Z

transforms/universal/hap/python/Makefile

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


These HAP_*VERSION require additions to .make.versions at the top of the repo.

I will make the update in .make.versions but going forward, we should change that: it could be enough to define it in Makefile itself.

fyi, at one point, maybe with kfp, these were required to be available more globally. In addition, the ray/Makefile refers to the HAP_PYTHON_VERSION, so may not be able to change unless we coalesce runtimes into a single project.

daw3rd · 2024-09-30T13:07:57Z

transforms/universal/hap/python/Makefile

+
+venv::	.transforms.python-venv
+
+install:: pip install -r requirements.txt


This is a non-standard rule. You should probably remove it.

daw3rd · 2024-09-30T13:38:48Z

transforms/universal/hap/python/test/test_hap.py

Can you also add a luancher-based tests, test_hap_python.py.

touma-I

@ian-cho We can go over my proposed changes together if any of it does not make sense.

.github/workflows/test-universal-hap.yml

transforms/universal/hap/python/Dockerfile

transforms/universal/hap/python/Makefile

transforms/universal/hap/python/requirements.txt

transforms/universal/hap/python/pyproject.toml

Co-authored-by: touma-I <[email protected]>

ian-cho · 2024-10-01T02:19:10Z

@blublinsky (cc @daw3rd )

Hi thanks for fixes! I merge your suggested commits. Several tests still failed and now I am investigating it. Please let me know if you have any ideas on the failed test. Thanks again.

All tests passed. Please check

transforms/universal/hap/python/pyproject.toml

touma-I · 2024-10-01T09:03:29Z

@ian-cho There was one typo in the pypropoject.toml that was causing the build to fix. This is fixed now as below

authors = [
 { name = "Ian Cho", email = "[email protected]" },
 ]
-[tool.setuptools.dynamic]
-dependencies = {file = ["requirements.txt"]}
+
+dynamic = ["dependencies"]

transforms/universal/hap/python/Dockerfile

…tory

transforms/universal/hap/python/src/hap_transform.py

daw3rd

Assuming issues to be created for remaining comments.

daw3rd · 2024-10-01T13:03:41Z

transforms/universal/hap/python/src/hap_transform.py

+        super().__init__(config)
+        self.model_name_or_path = config.get("model_name_or_path")
+        self.annotation_column = config.get("annotation_column")
+        self.doc_text_column = config.get("doc_text_column")


these should all have defaults or issue an exception if there is no default defined and it is not provided by the user. In general, we should assume (when possible), that the transform class may be instantiated directly by the user. This would mean the CLI is not used and so these could be empty key values.

@daw3rd agree, it is possible that users directly instantiate the class and there would be empty key values. How about self.model_name_or_path = config.get("model_name_or_path", ibm-granite/granite-guardian-hap-38m)? the same for other two parameters. I can add a warning message if the user does not specify a model, notifying them that the default model is triggered?

ian-cho added 26 commits September 25, 2024 01:19

Add files via upload

d7bb351

Update hap_local.py

d7da30f

Update hap_transform.py

890fbdc

Merge branch 'IBM:dev' into dev

a47dd93

Update hap_local_python.py

e4ff06f

Create README.md

5996da8

Update README.md

f848202

Update README.md

02b45df

Create requirements.txt

9df6920

Update README.md

4166f9d

Update README.md

b135c17

Add files via upload

b665b32

Update test_hap.py

5604355

Merge branch 'IBM:dev' into dev

35656bf

Add files via upload

add8478

Add files via upload

1add5d5

Update test_hap.py

9029752

Delete transforms/universal/hap/python/test/test_hap_python.py

e9324f3

Add files via upload

f792276

Add files via upload

d304b50

Update requirements.txt

e930abd

Add files via upload

748c740

Add files via upload

79948bf

Add files via upload

abf66e5

Merge branch 'IBM:dev' into dev

3c069f2

Add files via upload

22fba55

daw3rd requested changes Sep 30, 2024

View reviewed changes

transforms/universal/hap/python/test/test_hap.py Outdated

Copy link

Member

daw3rd Sep 30, 2024

Choose a reason for hiding this comment

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

Can you also add a luancher-based tests, test_hap_python.py.

touma-I requested changes Sep 30, 2024

View reviewed changes

touma-I reviewed Sep 30, 2024

View reviewed changes

transforms/universal/hap/python/pyproject.toml Outdated Show resolved Hide resolved

ian-cho and others added 11 commits October 1, 2024 10:26

Update transforms/universal/hap/python/pyproject.toml

ebc528f

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

9b9bfcc

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

50fa791

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

1a331ee

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

453acd3

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

d4c18d1

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

d28076c

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

4b1964a

Co-authored-by: touma-I <[email protected]>

Add files via upload

a79d4d4

Update transforms/universal/hap/python/pyproject.toml

f1f99f2

Co-authored-by: touma-I <[email protected]>

Update transforms/universal/hap/python/pyproject.toml

a19c158

Co-authored-by: touma-I <[email protected]>

Update hap_transform.py

041564b

touma-I reviewed Oct 1, 2024

View reviewed changes

transforms/universal/hap/python/pyproject.toml Outdated Show resolved Hide resolved

touma-I reviewed Oct 1, 2024

View reviewed changes

transforms/universal/hap/python/pyproject.toml Outdated Show resolved Hide resolved

touma-I added 2 commits October 1, 2024 04:54

Update transforms/universal/hap/python/pyproject.toml

5743128

Update transforms/universal/hap/python/pyproject.toml

fb2d6a0

touma-I reviewed Oct 1, 2024

View reviewed changes

transforms/universal/hap/python/Dockerfile Outdated Show resolved Hide resolved

touma-I and others added 4 commits October 1, 2024 05:04

Update transforms/universal/hap/python/Dockerfile

1f7986a

Add files via upload

36d262a

Add files via upload

b9a80cf

Delete transforms/universal/hap/python/test-data/input/expected direc…

d64d9bf

…tory

touma-I approved these changes Oct 1, 2024

View reviewed changes

touma-I reviewed Oct 1, 2024

View reviewed changes

transforms/universal/hap/python/src/hap_transform.py Outdated Show resolved Hide resolved

Update transforms/universal/hap/python/src/hap_transform.py

7284c81

daw3rd approved these changes Oct 1, 2024

View reviewed changes

touma-I merged commit 00d1fea into IBM:dev Oct 1, 2024
6 checks passed

shahrokhDaijavad mentioned this pull request Oct 2, 2024

A set of enhancements for the HAP tranform #659

Closed

2 tasks

touma-I mentioned this pull request Oct 3, 2024

Update HAP README.md #661

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding hap transform #638

adding hap transform #638

ian-cho commented Sep 28, 2024

daw3rd Sep 30, 2024

daw3rd Sep 30, 2024

touma-I Sep 30, 2024

daw3rd Oct 1, 2024

daw3rd Sep 30, 2024

daw3rd Sep 30, 2024

touma-I left a comment

ian-cho commented Oct 1, 2024 •

edited

Loading

touma-I commented Oct 1, 2024

daw3rd left a comment

daw3rd Oct 1, 2024

ian-cho Oct 3, 2024


		venv:: .transforms.python-venv

		install:: pip install -r requirements.txt

adding hap transform #638

adding hap transform #638

Conversation

ian-cho commented Sep 28, 2024

Why are these changes needed?

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

touma-I left a comment

Choose a reason for hiding this comment

ian-cho commented Oct 1, 2024 • edited Loading

touma-I commented Oct 1, 2024

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

ian-cho commented Oct 1, 2024 •

edited

Loading