Adding API to fetch tokenizer config for model #1052

kumar-shivam-ranjan · 2025-02-03T16:17:03Z

This PR intends to add additional model API to fetch tokenizer config of the model. It reads the tokenizer_config.json file from the model artifact location and returns the whole json as response body. The tokenizer config of the model contains chat template of the model which is useful when we deploy the model with chat completions endpoint.

Chat templates specify how to convert conversations, represented as lists of messages, into a single tokenizable string in the format that the model expects.

For models which don't have chat_templates, it will return empty json.

Request

curl --location 'http://localhost:8888/aqua/model/ocid1.datasciencemodel.oc1.iad.xxxx/tokenizer'

Response

{
    "bos_token": "<s>",
    "clean_up_tokenization_spaces": true,
    "cls_token": "<s>",
    "eos_token": "</s>",
    "mask_token": {
        "__type": "AddedToken",
        "content": "<mask>",
        "lstrip": true,
        "normalized": true,
        "rstrip": false,
        "single_word": false
    },
    "model_max_length": 8192,
    "pad_token": "<pad>",
    "sep_token": "</s>",
    "sp_model_kwargs": {},
    "tokenizer_class": "XLMRobertaTokenizer",
    "unk_token": "<unk>"
}

Unit tests

github-actions · 2025-02-03T16:49:48Z

📌 Cov diff with main:

📌 Overall coverage:

VipulMascarenhas

added some comments. Also, could you please add some unit tests as well?

VipulMascarenhas · 2025-02-03T16:40:10Z

ads/aqua/app.py

@@ -328,6 +328,55 @@ def get_config(self, model_id: str, config_file_name: str) -> Dict:

        return config

+    def get_chat_template(self, model_id):


can't we do similar to deployment and fine-tuning?

AQUA_MODEL_TOKENIZER_CONFIG = "tokenizer_config.json" def get_hf_tokenizer_config(self, model_id: str) -> Dict: config = self.get_config(model_id, AQUA_MODEL_TOKENIZER_CONFIG) if not config: logger.debug( f"Tokenizer config for model: {model_id} is not available. Use defaults." ) return config

Relevant to this, for verified models, we want to load the config from the service model bucket instead of user bucket. PR- #1053

If we use get_config directly, the above PR should take care of it.

VipulMascarenhas · 2025-02-03T16:40:55Z

ads/aqua/app.py

@@ -328,6 +328,55 @@ def get_config(self, model_id: str, config_file_name: str) -> Dict:

        return config

+    def get_chat_template(self, model_id):


add @telemetry decorator.

VipulMascarenhas · 2025-02-03T16:49:33Z

ads/aqua/extension/model_handler.py

@@ -316,8 +317,24 @@ def post(self, *args, **kwargs):  # noqa: ARG002
        )


+class AquaModelChatTemplateHandler(AquaAPIhandler):


it might be good to return the entire config instead of only the chat template field since there might be use cases down the line where other values might be needed from the tokenizer. UI can parse the json and get the chat_template.
We can name this AquaModelTokenizerConfigHandler accordingly.

github-actions · 2025-02-04T13:21:27Z

📌 Cov diff with main:

📌 Overall coverage:

VipulMascarenhas

changes look good. requesting one update to the endpoint name to match what we're returning.

VipulMascarenhas · 2025-02-04T13:47:05Z

ads/aqua/extension/model_handler.py

 __handlers__ = [
    ("model/?([^/]*)", AquaModelHandler),
    ("model/?([^/]*)/license", AquaModelLicenseHandler),
+    ("model/?([^/]*)/chat_template", AquaModelTokenizerConfigHandler),


let's change the endpoint name to /tokenizer instead of /chat_template since we'll be returning the entire json file contents.

github-actions · 2025-02-04T14:07:04Z

📌 Cov diff with main:

📌 Overall coverage:

…m/oracle/accelerated-data-science into feature/default-chat-template-api

github-actions · 2025-02-04T14:35:22Z

📌 Cov diff with main:

📌 Overall coverage:

github-actions · 2025-02-04T19:39:51Z

📌 Cov diff with main:

📌 Overall coverage:

mrDzurb · 2025-02-04T21:06:39Z

ads/aqua/app.py

@@ -306,20 +314,22 @@ def get_config(self, model_id: str, config_file_name: str) -> Dict:
            )
            base_model = self.ds_client.get_model(base_model_ocid).data
            artifact_path = get_artifact_path(base_model.custom_metadata_list)
+            if config_folder == "artifact":


It would be better to move strings to constants.

mrDzurb · 2025-02-04T21:08:33Z

ads/aqua/app.py

+        self,
+        model_id: str,
+        config_file_name: str,
+        config_folder: Optional[str] = "config",


What if to pass None as a config_folder? I guess in the code we should add something like

config_folder = config_folder or DEFAULT_MODEL_CONFIG_FOLDER

Let's also move the strings to the constants.

DEFAULT_MODEL_CONFIG_FOLDER = "config"

mrDzurb · 2025-02-04T21:09:54Z

ads/aqua/app.py

@@ -277,6 +282,9 @@ def get_config(self, model_id: str, config_file_name: str) -> Dict:
            The OCID of the Aqua model.
        config_file_name: str
            name of the config file
+        config_folder: Optional[str]


Let's add a default value to the docstring as well.

mrDzurb · 2025-02-04T21:11:05Z

ads/aqua/app.py

@@ -306,20 +314,22 @@ def get_config(self, model_id: str, config_file_name: str) -> Dict:
            )
            base_model = self.ds_client.get_model(base_model_ocid).data
            artifact_path = get_artifact_path(base_model.custom_metadata_list)
+            if config_folder == "artifact":


We might need to convert the config_folder to the lowercase?

config_folder values comes from constant enums which is always lowercase. so probably no need to convert explicitly?

Partially true, however we allow to pass a custom config_folder to the function?

mrDzurb · 2025-02-04T21:13:26Z

ads/aqua/app.py

        if not artifact_path:
            logger.debug(
                f"Failed to get artifact path from custom metadata for the model: {model_id}"
            )
            return config

-        config_path = f"{os.path.dirname(artifact_path)}/config/"
+        config_path = f"{os.path.dirname(artifact_path)}/{config_folder}/"


What if the config_folder will have / at the end? It might be better to use os.path.join?

config_folder is parameter that we pass to get_config function.
This determines whether we need to search for a particular file in artifact folder (like tokenizer_config.json) or config folder (like ft_config.json).
The value of the config folder will always be either "config" (default) or "artifact".

agree with DC, makes it easier to handle paths - something like
os.path.join(os.path.dirname(artifact_path), config_folder)?

mrDzurb · 2025-02-04T21:18:22Z

ads/aqua/extension/model_handler.py

+        paths = url_parse.path.strip("/")
+        path_list = paths.split("/")
+        if (
+            len(path_list) == 4


What is 4 and 3 in this code? :) Let's move them either to constants or add some meaningful comments.

The request path here is: /aqua/models/ocid1.iad.ahdxxx/tokenizer
path_list=['aqua','models','ocid1.iad.ahdxxx','tokenizer']

Added comments

mrDzurb

Hi @kumar-shivam-ranjan, could you also change the title and description for the PR, since the scope of the PR has been adjusted.

kumar-shivam-ranjan · 2025-02-05T07:50:04Z

Hi @kumar-shivam-ranjan, could you also change the title and description for the PR, since the scope of the PR has been adjusted.

Updated

github-actions · 2025-02-05T08:16:08Z

📌 Cov diff with main:

📌 Overall coverage:

VipulMascarenhas

minor comments, rest looks good.

ads/aqua/app.py

VipulMascarenhas · 2025-02-05T10:52:39Z

ads/aqua/app.py

        if not artifact_path:
            logger.debug(
                f"Failed to get artifact path from custom metadata for the model: {model_id}"
            )
            return config

-        config_path = f"{os.path.dirname(artifact_path)}/config/"
+        config_path = f"{os.path.dirname(artifact_path)}/{config_folder}/"


agree with DC, makes it easier to handle paths - something like
os.path.join(os.path.dirname(artifact_path), config_folder)?

ads/aqua/app.py

github-actions · 2025-02-05T11:01:02Z

📌 Cov diff with main:

📌 Overall coverage:

github-actions · 2025-02-05T12:53:28Z

📌 Cov diff with main:

📌 Overall coverage:

VipulMascarenhas

lgtm 👍

mrDzurb · 2025-02-05T23:11:05Z

ads/aqua/extension/model_handler.py

+    def get(self, model_id):
+        url_parse = urlparse(self.request.path)
+        paths = url_parse.path.strip("/")
+        path_list = paths.split("/")


NIT: Looks like we only use path_list further in this method, then probably can do

path_list = urlparse(self.request.path).path.strip("/").split("/")

mrDzurb · 2025-02-05T23:11:49Z

ads/aqua/extension/model_handler.py

@@ -316,8 +317,26 @@ def post(self, *args, **kwargs):  # noqa: ARG002
        )


+class AquaModelTokenizerConfigHandler(AquaAPIhandler):


Let's add the pydocs to the class?

Handles requests for retrieving the Hugging Face tokenizer configuration of a specified model. Expected request format: GET /aqua/models/<model-ocid>/tokenizer

mrDzurb · 2025-02-05T23:14:55Z

ads/aqua/extension/model_handler.py

+            and path_list[3] == "tokenizer"
+        ):
+            return self.finish(AquaModelApp().get_hf_tokenizer_config(model_id))
+        else:


NIT: looks like else is not required here. I think it would be more clear to do something like this:

is_valid_path = ( len(path_list) == 4 and path_list[3] == "tokenizer" and is_valid_ocid(path_list[2]) ) if not is_valid_path: raise HTTPError(400, f"The request {self.request.path} is invalid.") return self.finish(AquaModelApp().get_hf_tokenizer_config(model_id))

github-actions · 2025-02-06T07:11:56Z

📌 Cov diff with main:

📌 Overall coverage:

github-actions · 2025-02-06T10:16:17Z

📌 Cov diff with main:

📌 Overall coverage:

kumar-shivam-ranjan added 4 commits February 3, 2025 19:06

Adding default chat_template api

707e96c

Merge branch 'main' into feature/default-chat-template-api

30592c8

Adding chat template api

3cc7e8a

Adding chat template api

41a305c

kumar-shivam-ranjan requested review from darenr, mayoor, mrDzurb, VipulMascarenhas, qiuosier and ahosler as code owners February 3, 2025 16:17

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 3, 2025

Adding chat template api

b95aeb6

VipulMascarenhas requested changes Feb 3, 2025

View reviewed changes

kumar-shivam-ranjan added 2 commits February 4, 2025 16:48

Merge branch 'main' into feature/default-chat-template-api

74601c0

Addressing review comments

8997594

kumar-shivam-ranjan added 2 commits February 4, 2025 19:10

Formatting

dbed1dd

Merge branch 'main' into feature/default-chat-template-api

aa0ee81

VipulMascarenhas reviewed Feb 4, 2025

View reviewed changes

Updating path

d6b728e

Merge branch 'feature/default-chat-template-api' of https://github.co…

43f94b0

…m/oracle/accelerated-data-science into feature/default-chat-template-api

VipulMascarenhas previously approved these changes Feb 4, 2025

View reviewed changes

Resolving conflicts

035c4ee

kumar-shivam-ranjan dismissed VipulMascarenhas’s stale review via 035c4ee February 4, 2025 19:13

mrDzurb reviewed Feb 4, 2025

View reviewed changes

mrDzurb reviewed Feb 5, 2025

View reviewed changes

Addressing review comments

a5cb572

kumar-shivam-ranjan changed the title ~~Adding API to fetch default chat template for models~~ Adding API to fetch tokenizer config for model Feb 5, 2025

Merge branch 'main' into feature/default-chat-template-api

766b201

Fixing tests

8356379

VipulMascarenhas reviewed Feb 5, 2025

View reviewed changes

kumar-shivam-ranjan added 3 commits February 5, 2025 17:52

Addressing review comments

e449f0a

Addressing review comments

ef2c40e

Addressing review comments

88fa1cc

kumar-shivam-ranjan requested review from VipulMascarenhas and mrDzurb February 5, 2025 12:29

VipulMascarenhas previously approved these changes Feb 5, 2025

View reviewed changes

kumar-shivam-ranjan self-assigned this Feb 5, 2025

mrDzurb previously approved these changes Feb 5, 2025

View reviewed changes

Addressing review comments

c66bdd6

kumar-shivam-ranjan dismissed stale reviews from mrDzurb and VipulMascarenhas via c66bdd6 February 6, 2025 06:46

kumar-shivam-ranjan requested review from VipulMascarenhas and mrDzurb February 6, 2025 06:48

kumar-shivam-ranjan added 2 commits February 6, 2025 15:09

Adding model_id in MD response

a2f0ebe

Fixing UTs

be96c70

VipulMascarenhas approved these changes Feb 6, 2025

View reviewed changes

mrDzurb approved these changes Feb 6, 2025

View reviewed changes

kumar-shivam-ranjan merged commit 0db36f2 into main Feb 6, 2025
21 checks passed

		@@ -328,6 +328,55 @@ def get_config(self, model_id: str, config_file_name: str) -> Dict:

		return config

		def get_chat_template(self, model_id):

		@@ -316,8 +317,24 @@ def post(self, args, *kwargs): # noqa: ARG002
		)


		class AquaModelChatTemplateHandler(AquaAPIhandler):

		@@ -316,8 +317,26 @@ def post(self, args, *kwargs): # noqa: ARG002
		)


		class AquaModelTokenizerConfigHandler(AquaAPIhandler):

Adding API to fetch tokenizer config for model #1052

Adding API to fetch tokenizer config for model #1052

Conversation

kumar-shivam-ranjan commented Feb 3, 2025 • edited Loading

Unit tests

github-actions bot commented Feb 3, 2025

VipulMascarenhas 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

github-actions bot commented Feb 4, 2025

VipulMascarenhas left a comment

Choose a reason for hiding this comment

VipulMascarenhas Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

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

kumar-shivam-ranjan Feb 5, 2025 • edited Loading

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

mrDzurb left a comment

Choose a reason for hiding this comment

kumar-shivam-ranjan commented Feb 5, 2025

github-actions bot commented Feb 5, 2025

VipulMascarenhas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 5, 2025

github-actions bot commented Feb 5, 2025

VipulMascarenhas 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

github-actions bot commented Feb 6, 2025

github-actions bot commented Feb 6, 2025

kumar-shivam-ranjan commented Feb 3, 2025 •

edited

Loading

VipulMascarenhas Feb 4, 2025 •

edited

Loading

kumar-shivam-ranjan Feb 5, 2025 •

edited

Loading