Add description field with make_model_config_json function #203

thanawan-atc · 2023-08-01T18:19:34Z

Description

Enable make_model_config_json function to add description field to model config file

Check List

New functionality includes testing.
- All tests pass
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

codecov · 2023-08-01T20:15:30Z

Codecov Report

Merging #203 (f5f7552) into main (1237aa6) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
+ Coverage   91.15%   91.23%   +0.08%     
==========================================
  Files          37       37              
  Lines        4092     4130      +38     
==========================================
+ Hits         3730     3768      +38     
  Misses        362      362

Files Changed	Coverage Δ
...search_py_ml/ml_models/sentencetransformermodel.py	`73.93% <100.00%> (+2.57%)`	⬆️

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

dhrubo-os · 2023-08-04T18:11:40Z

CHANGELOG.md

@@ -29,6 +29,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Make make_model_config_json function more concise by @thanawan-atc in ([#191](https://github.com/opensearch-project/opensearch-py-ml/pull/191))
 - Enabled auto-truncation for any pretrained models by @Yerzhaisang in ([#192](https://github.com/opensearch-project/opensearch-py-ml/pull/192))
 - Generalize make_model_config_json function by @thanawan-atc in ([#200](https://github.com/opensearch-project/opensearch-py-ml/pull/200))
+- Enable make_model_config_json to add model description to model config file by @thanawan-atc in ([#203](https://github.com/opensearch-project/opensearch-py-ml/pull/203))


1.1.0 is already released. Let's add another section for 1.2.0?

dhrubo-os · 2023-08-04T18:14:53Z

tests/conftest.py

@@ -109,7 +109,7 @@ def check_values(self, oml_obj, pd_obj):

    def check_exception(self, ed_exc, pd_exc):
        """Checks that either an exception was raised or not from both opensearch_py_ml and pandas"""
-        assert (ed_exc is None) == (pd_exc is None) and type(ed_exc) == type(pd_exc)
+        assert (ed_exc is None) == (pd_exc is None) and isinstance(ed_exc, type(pd_exc))


what is this for? Just curious to know why it's failing now?

I tried to look at code history, but also couldn't figure out why it's failing now. But it says in the error message that we can't do type(xxx) == type(xxx).

Did you check if it's passing all the tests?

It passes the integration test. So, doesn’t it mean that it passes all? Also, one strange thing is when I run nox -s lint on Mac and CloudDevDesktop, it doesn’t show the warning. It only failed on GitHub workflow.

Here’s the error

dhrubo-os · 2023-08-04T18:15:22Z

tests/ml_commons/test_ml_commons_client.py

@@ -68,8 +68,8 @@ def clean_test_folder(TEST_FOLDER):


 def test_init():
-    assert type(ml_client._client) == OpenSearch
-    assert type(ml_client._model_uploader) == ModelUploader
+    assert isinstance(ml_client._client, OpenSearch)


Same question here.

Same error here.

dhrubo-os · 2023-08-07T22:38:36Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

+        readme_data = MarkDownFile.read_file(readme_file_path)
+        start_str = f"# {self.model_id}"
+        start = readme_data.find(start_str)
+        if start == -1:


What does this mean? Can we add a comment here? Let's add a detailed description about this parsing logic.

dhrubo-os · 2023-08-07T22:39:56Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

@@ -1006,6 +1008,36 @@ def set_up_accelerate_config(
                "Failed to open config file for ml common upload: " + file_path + "\n"
            )

+    def get_model_description_from_md_file(self, readme_file_path) -> str:


as this is an internal function, let's add _ in front of it.

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

dhrubo-os · 2023-08-08T20:22:10Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

+            model_name = self.model_id.split("/")[1]
+            start_str = f"# {model_name}"
+            start = readme_data.find(start_str)
+        end = readme_data.find("## ", start)


Did you try with multiple sentence transformer models, how does this work. I feel like this is brittle. But I understand for the scraping we might need to depend on some placeholder. I just wanted to make sure if you checked with multiple different models?

I randomly tried with many different models and it works. I can also add that it has to start with the word "This is". All the models I have seen use this format.

Talked to Dhrubo offline and ended up not checking "This is" and made edits in the section ending part instead.

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

dhrubo-os · 2023-08-08T23:13:05Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

@@ -1035,7 +1035,7 @@ def _get_model_description_from_md_file(self, readme_file_path) -> str:
            model_name = self.model_id.split("/")[1]
            start_str = f"# {model_name}"
            start = readme_data.find(start_str)
-        end = readme_data.find("## ", start)
+        end = readme_data.find("# ", start + len(start_str))


This approach has a potential issue: if there are other occurrences of # within the description section that are not intended to be section headers, it might lead to incorrect results. For example, if the description contains a sentence like "This model has an accuracy of 90% # which is impressive", the code might incorrectly identify that # as the start of a new section.

# Find the index of the next line that starts with a '#' end = readme_data.find('\n# ', start + len(start_str))

What do you think about this?

Talked to Dhrubo offline and decided to use end = readme_data.find("\n#", start + len(start_str)) with no spacebar after #. Note that clients can always overwrite description and we can do similar thing with the GitHub Actions workflow.

ylwu-amzn · 2023-08-08T23:43:45Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

@@ -1006,6 +1008,55 @@ def set_up_accelerate_config(
                "Failed to open config file for ml common upload: " + file_path + "\n"
            )

+    def _get_model_description_from_md_file(self, readme_file_path) -> str:


Fromt the comment below,

Get description of the model from README.md file

Seems we should rename the method name as _get_model_description_from_readme_file

ylwu-amzn · 2023-08-08T23:44:36Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

@@ -1006,6 +1008,55 @@ def set_up_accelerate_config(
                "Failed to open config file for ml common upload: " + file_path + "\n"
            )

+    def _get_model_description_from_md_file(self, readme_file_path) -> str:
+        """
+        Get description of the model from README.md file


Is the README.md file from huggingface? Can you add some example link?

Yes, it's from huggingface. I can add example.

ylwu-amzn · 2023-08-08T23:46:56Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

+        description = re.sub(r"\*", "", description)
+
+        # Remove unnecessary part if exists(e.g. " For an introduction to ..."
+        unnecessary_part = description.find(" For an introduction to")


I'm not sure how general this description method. Is the readme file always follow such pattern with " For an introduction to" ?

If the readme pattern is unknown, or it's hard to predict the pattern. We'd better make the method flexible or more general for future pattern

There are couple models that have this "For an introduction to ... " line (https://huggingface.co/sentence-transformers/multi-qa-mpnet-base-dot-v1), so I remove it if it exists since Dhrubo manually removes it from the current model listing. It will do nothing if it doesn't exist though.

ylwu-amzn · 2023-08-08T23:50:06Z

opensearch_py_ml/ml_models/sentencetransformermodel.py

@@ -1126,6 +1193,9 @@ def make_model_config_json(
            },
        }

+        if description is not None:
+            model_config_content["description"] = description


If description is None, can we add some general description? For example "This is a sentence transformer model which can convert text into embedding (float vector)"?

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

ylwu-amzn

LGTM. Thanks!

* Add description field Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Restore notebook Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Resolve linting issues Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update CHANGELOG.md Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test_sentencetransformermodel_pytest.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve test coverage Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Edit test name Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change CHANGELOG.md & Add comment to sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve add description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Loosen restriction Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change function name + Add comment + Add default description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> --------- Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> (cherry picked from commit 20435b1)

* updating notebook + bumping version (#199) * updating notebook + bumping version Signed-off-by: Dhrubo Saha <[email protected]> * addressing comments Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit 1237aa6) * Add description field with make_model_config_json function (#203) * Add description field Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Restore notebook Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Resolve linting issues Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update CHANGELOG.md Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test_sentencetransformermodel_pytest.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve test coverage Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Edit test name Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change CHANGELOG.md & Add comment to sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve add description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Loosen restriction Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change function name + Add comment + Add default description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> --------- Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> (cherry picked from commit 20435b1) --------- Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> Co-authored-by: Dhrubo Saha <[email protected]>

* updating notebook + bumping version (#199) * updating notebook + bumping version Signed-off-by: Dhrubo Saha <[email protected]> * addressing comments Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit 1237aa6) * Add description field with make_model_config_json function (#203) * Add description field Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Restore notebook Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Resolve linting issues Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update CHANGELOG.md Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test_sentencetransformermodel_pytest.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve test coverage Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Edit test name Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change CHANGELOG.md & Add comment to sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve add description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Loosen restriction Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change function name + Add comment + Add default description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> --------- Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> (cherry picked from commit 20435b1) * Backport Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Fix linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update test_sentencetransformermodel_pytest.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> --------- Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> Co-authored-by: Dhrubo Saha <[email protected]>

* updating notebook + bumping version (#199) * updating notebook + bumping version Signed-off-by: Dhrubo Saha <[email protected]> * addressing comments Signed-off-by: Dhrubo Saha <[email protected]> --------- Signed-off-by: Dhrubo Saha <[email protected]> (cherry picked from commit 1237aa6) * Add description field with make_model_config_json function (#203) * Add description field Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Restore notebook Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Resolve linting issues Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update CHANGELOG.md Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug test_sentencetransformermodel_pytest.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve test coverage Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Edit test name Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change CHANGELOG.md & Add comment to sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Improve add description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Loosen restriction Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Update sentencetransformermodel.py Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Change function name + Add comment + Add default description Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Debug Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> --------- Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> (cherry picked from commit 20435b1) * Resolve merged conflict Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> * Correct CHANGELOG.md Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> --------- Signed-off-by: Thanawan Atchariyachanvanit <[email protected]> Co-authored-by: Dhrubo Saha <[email protected]>

Add description field

5489701

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

thanawan-atc requested review from dhrubo-os, greaa-aws, ylwu-amzn, b4sjoo, jngz-es and rbhavna as code owners August 1, 2023 18:19

thanawan-atc added 2 commits August 1, 2023 11:37

Restore notebook

a01b458

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Debug test

2d48813

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

thanawan-atc force-pushed the add_description_config_json branch from 09e9da3 to 2d48813 Compare August 1, 2023 19:06

thanawan-atc added 3 commits August 1, 2023 12:07

Resolve linting issues

4543513

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Update CHANGELOG.md

67f592d

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Debug test_sentencetransformermodel_pytest.py

6a9faf3

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

thanawan-atc added 2 commits August 1, 2023 23:01

Improve test coverage

e451d06

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Edit test name

ed2ea6f

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

thanawan-atc mentioned this pull request Aug 4, 2023

[Draft for Initial Review] Automating Model Autotracing and Uploading Process #204

Closed

dhrubo-os reviewed Aug 7, 2023

View reviewed changes

thanawan-atc added 2 commits August 7, 2023 23:00

Change CHANGELOG.md & Add comment to sentencetransformermodel.py

a3d5f1d

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Correct linting

8fec824

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

dhrubo-os reviewed Aug 8, 2023

View reviewed changes

thanawan-atc added 4 commits August 8, 2023 14:56

Improve add description

a307d41

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Correct linting

489a921

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Loosen restriction

04ff9f5

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Update sentencetransformermodel.py

2e33d07

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

dhrubo-os reviewed Aug 8, 2023

View reviewed changes

thanawan-atc requested a review from dhrubo-os August 8, 2023 23:33

dhrubo-os approved these changes Aug 8, 2023

View reviewed changes

ylwu-amzn reviewed Aug 8, 2023

View reviewed changes

thanawan-atc added 2 commits August 8, 2023 17:12

Change function name + Add comment + Add default description

a39d9e9

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

Debug

f5f7552

Signed-off-by: Thanawan Atchariyachanvanit <[email protected]>

thanawan-atc requested a review from ylwu-amzn August 9, 2023 04:46

ylwu-amzn approved these changes Aug 9, 2023

View reviewed changes

dhrubo-os approved these changes Aug 9, 2023

View reviewed changes

dhrubo-os merged commit 20435b1 into opensearch-project:main Aug 9, 2023
14 checks passed

dhrubo-os added the backport 1.x label Aug 9, 2023

thanawan-atc mentioned this pull request Aug 10, 2023

Backport #203 to 1.x #216

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add description field with make_model_config_json function #203

Add description field with make_model_config_json function #203

thanawan-atc commented Aug 1, 2023 •

edited

Loading

codecov bot commented Aug 1, 2023 •

edited

Loading

dhrubo-os Aug 4, 2023

dhrubo-os Aug 4, 2023

thanawan-atc Aug 8, 2023

dhrubo-os Aug 8, 2023

thanawan-atc Aug 8, 2023

thanawan-atc Aug 8, 2023

dhrubo-os Aug 4, 2023

thanawan-atc Aug 8, 2023

dhrubo-os Aug 7, 2023

dhrubo-os Aug 7, 2023

dhrubo-os Aug 8, 2023

thanawan-atc Aug 8, 2023

thanawan-atc Aug 8, 2023

dhrubo-os Aug 8, 2023

thanawan-atc Aug 8, 2023

ylwu-amzn Aug 8, 2023

thanawan-atc Aug 8, 2023

ylwu-amzn Aug 8, 2023

thanawan-atc Aug 8, 2023

ylwu-amzn Aug 8, 2023

ylwu-amzn Aug 8, 2023

thanawan-atc Aug 8, 2023

ylwu-amzn Aug 8, 2023

thanawan-atc Aug 8, 2023

ylwu-amzn left a comment

Add description field with make_model_config_json function #203

Add description field with make_model_config_json function #203

Conversation

thanawan-atc commented Aug 1, 2023 • edited Loading

Description

Check List

codecov bot commented Aug 1, 2023 • edited Loading

Codecov Report

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

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

ylwu-amzn left a comment

Choose a reason for hiding this comment

thanawan-atc commented Aug 1, 2023 •

edited

Loading

codecov bot commented Aug 1, 2023 •

edited

Loading