Make make_model_config_json function more concise
#191
Conversation
Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com>
thanawan-atc
requested review from
dhrubo-os,
greaa-aws,
ylwu-amzn,
b4sjoo,
jngz-es and
rbhavna
as code owners
Codecov Report
@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 90.96% 91.06% +0.10%
==========================================
Files 37 37
Lines 4073 4052 -21
==========================================
- Hits 3705 3690 -15
+ Misses 368 362 -6
|
| if normalize_result is not None: | ||
| model_config_content["model_config"]["normalize_result"] = normalize_result | ||
| else: | ||
| normalize_result_json_file_path = os.path.join(folder_path, "2_Normalize") | ||
| if os.path.exists(normalize_result_json_file_path): | ||
| if len(model._modules) >= 3 and isinstance(model._modules["2"], Normalize): |
There was a problem hiding this comment.
I think otherwise we can say normalize_result = False?
| model_type = model._modules["0"].auto_model.__class__.__name__ | ||
| model_type = model_type.lower().rstrip("model") | ||
| except Exception as e: | ||
| raise Exception(f"Raised exception while getting model_type: {e}") |
There was a problem hiding this comment.
Let's try to cover these exception branches. I think either we can mock the model or we can stub the _modules as None so that it can go to the except branch. Same for other exceptions.
There was a problem hiding this comment.
I don't think we can do it that way because the function loads a hugging face SentenceTransformer within the function, so we cannot stub the _modules as None in test file.
There was a problem hiding this comment.
I agree this will be bit tricky to implement. We can skip this for now. Not urgent.
Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com>
Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com>
thanawan-atc
force-pushed
the
edit-make-model-config-json
branch
from
a254149 to
33940bb
Compare
Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com>
Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com>
Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com>
| # if user input model_type and embedding_dimension, it will skip reading the config.json file | ||
| if model_type is None or embedding_dimension is None: | ||
| # if user input model_type/embedding_dimension/pooling_mode, it will skip this step. | ||
| model = SentenceTransformer(self.model_id) |
There was a problem hiding this comment.
can we use model_name here? Customer might provide another model name here
There was a problem hiding this comment.
I can change it to be model_name, but model_name still needs to be hugging face model_id for it to work though (i.e., 'sentence-transformers/msmarco-distilbert-base-tas-b').
I was also wondering why we need to have model_name. I thought that we just want to allow user to set the arbitrary name for the model. But in that case line 1026 should still be self.model_id because SentenceTransformer("Arbitrary Name") would not work [Note that it is hugging face class SentenceTransformer not our class SentenceTransformerModel here.] I'm not sure if I misunderstand anything. Let me know if you still want me to change it to model_name.
There was a problem hiding this comment.
You were right. We can't use model_name. For some reason I mixed up with model_name with model_id. Ignore my comment.
* Edit make_model_config_json function Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Improve cov Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Improve cov Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Update CHANGELOG.md Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Improve cov (2) Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> --------- Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> (cherry picked from commit 7622af5)
* Edit make_model_config_json function Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Improve cov Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Improve cov Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Update CHANGELOG.md Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Improve cov (2) Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> * Correct linting Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> --------- Signed-off-by: Thanawan Atchariyachanvanit <latchari@amazon.com> (cherry picked from commit 7622af5) Co-authored-by: Thanawan Atchariyachanvanit <latchari@amazon.com>
Description
Instead of scrapping
embedding_dimension,pooling_mode, andnormalize_resultfrom config.json file, we can use sentence transformer attributes and build-in function directly.Check List
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.