Skip to content

feat: enhance tokenizer handling in model configuration - #147

Open
0x2f0713 wants to merge 1 commit into
NotPunchnox:mainfrom
0x2f0713:main
Open

feat: enhance tokenizer handling in model configuration#147
0x2f0713 wants to merge 1 commit into
NotPunchnox:mainfrom
0x2f0713:main

Conversation

@0x2f0713

@0x2f0713 0x2f0713 commented Apr 25, 2026

Copy link
Copy Markdown

PR Summary

What changed

  • Updated server.py so rkllama_client pull now performs tokenizer repo detection during model download.
  • Added a new tokenizer discovery path in /pull:
    • check whether the provided Hugging Face repo contains config.json
    • if it does, generate Modelfile without TOKENIZER
    • if it does not, walk finetune/base-model metadata to resolve a repository that does contain config.json
    • if an upstream repo is found, write TOKENIZER="<resolved_repo>" into Modelfile

Why this change was needed

  • Some RKLLM-converted model repos (like randomblock1/...-rk3588) do not include a valid Hugging Face tokenizer config.
  • Previously, the pull flow always assumed the source repo could be used for tokenizer resolution, which caused failures in AutoTokenizer.from_pretrained(...).
  • This led to broken chat loading unless users manually added TOKENIZER to the Modelfile.

Benefits

  • Automatically avoids unnecessary TOKENIZER entries for repos that already include config.json.
  • Automatically resolves the correct tokenizer source for finetuned RKLLM repos that inherit from a base HF model.
  • Reduces manual configuration and makes the pull workflow more reliable and user-friendly.
  • Preserves existing TOKENIZER fallback behavior for models that truly need it.

Files changed

  • server.py
  • server_utils.py

Behavior now

  • rkllama_client pull owner/repo/file.rkllm:
    • if owner/repo/config.json exists → Modelfile uses only FROM + HUGGINGFACE_PATH
    • else → attempt to identify a parent/base repo with config.json
    • if found → Modelfile includes TOKENIZER="<resolved_repo>"
    • if not found → Modelfile is created without TOKENIZER and the failure is explicit in logs

Notes

  • No change was required in the existing tokenizer loading path in server_utils.py; it already supports TOKENIZER fallback.
  • This fix is focused on model pull behavior and modelfile generation, not on runtime inference logic.

Co-authored-by: Copilot copilot@github.com

Co-authored-by: Copilot <copilot@github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Enhances the model pull workflow so RKLLM-converted Hugging Face repos that lack tokenizer/config artifacts can automatically resolve and record an upstream tokenizer source in the generated Modelfile, improving reliability of tokenizer loading at runtime.

Changes:

  • Extend Modelfile generation to optionally include TOKENIZER="<resolved_repo>".
  • Add Hugging Face repo inspection + metadata-walk logic during /pull to resolve a repo that contains config.json.
  • Update tokenizer loading to prefer TOKENIZER from Modelfile and fall back to HUGGINGFACE_PATH.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
src/rkllama/server/server.py Adds tokenizer repo discovery during /pull and writes TOKENIZER into Modelfile when needed.
src/rkllama/api/server_utils.py Adjusts tokenizer loading to use TOKENIZER first, with fallback + clearer failure behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +114 to +120
def hf_repo_has_config(repo: str, fs=None) -> bool:
fs = fs or HfFileSystem()
try:
fs.info(f"{repo}/config.json")
return True
except Exception:
return False

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

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

hf_repo_has_config catches all exceptions and returns False, which can silently treat auth/network failures as “missing config.json”. Consider only returning False for “not found” errors (e.g., EntryNotFound/FileNotFound) and logging (or re-raising) unexpected exceptions so pull output/logs are actionable when Hugging Face is unavailable or credentials are missing.

Copilot uses AI. Check for mistakes.
Comment on lines +123 to +126
def get_hf_model_metadata(repo: str) -> dict | None:
try:
response = requests.get(f"https://huggingface.co/api/models/{repo}", timeout=10)
if response.status_code == 200:

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

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

The return type annotations use PEP 604 unions (e.g., dict | None, str | None), but the project metadata declares requires-python >=3.9. Python 3.9 will fail to import this module due to syntax error. Either switch these annotations to Optional[...] / Union[...] or bump the project’s minimum supported Python version to 3.10+.

Copilot uses AI. Check for mistakes.
Comment on lines +126 to +128
if response.status_code == 200:
return response.json()
except Exception as e:

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

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

get_hf_model_metadata returns None for any non-200 response without logging the status/body, which makes tokenizer resolution failures hard to diagnose (e.g., 404 vs 401 vs rate limit). Consider logging at least the status code (debug/info) and/or using response.raise_for_status() with controlled handling.

Suggested change
if response.status_code == 200:
return response.json()
except Exception as e:
response.raise_for_status()
try:
return response.json()
except ValueError as e:
logger.debug(f"Unable to parse Hugging Face metadata JSON for {repo}: {e}")
except requests.HTTPError as e:
response = e.response
status_code = response.status_code if response is not None else "unknown"
response_body = ""
if response is not None:
response_body = response.text[:500].replace("\n", "\\n")
logger.debug(
f"Unable to load Hugging Face metadata for {repo}: HTTP {status_code}"
+ (f" - {response_body}" if response_body else "")
)
except requests.RequestException as e:

Copilot uses AI. Check for mistakes.
Comment on lines +387 to +390
if hf_repo_has_config(repo):
yield f"Repository {repo} contains config.json; TOKENIZER not required.\n"
else:
tokenizer_repo = resolve_tokenizer_repo(repo)

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

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

In /pull, you already instantiate fs = HfFileSystem() for the file download metadata, but hf_repo_has_config(repo) / resolve_tokenizer_repo(repo) create their own HfFileSystem instances. Pass the existing fs into these helpers to avoid redundant client setup and extra remote calls (and to ensure consistent auth/caching behavior).

Suggested change
if hf_repo_has_config(repo):
yield f"Repository {repo} contains config.json; TOKENIZER not required.\n"
else:
tokenizer_repo = resolve_tokenizer_repo(repo)
try:
repo_has_config = hf_repo_has_config(repo, fs=fs)
except TypeError:
repo_has_config = hf_repo_has_config(repo)
if repo_has_config:
yield f"Repository {repo} contains config.json; TOKENIZER not required.\n"
else:
try:
tokenizer_repo = resolve_tokenizer_repo(repo, fs=fs)
except TypeError:
tokenizer_repo = resolve_tokenizer_repo(repo)

Copilot uses AI. Check for mistakes.
@@ -93,11 +93,23 @@ def get_tokenizer(model_name):
logger.debug("Local Tokenizer doesn't exists!")

# Get model specific tokenizer from Huggin Face specified in Modelfile

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

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

Spelling in the comment: “Huggin Face” should be “Hugging Face”.

Suggested change
# Get model specific tokenizer from Huggin Face specified in Modelfile
# Get model specific tokenizer from Hugging Face specified in Modelfile

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants