Skip to content

feat: support parsing dtype field from config.#1118

Open
DongheJin wants to merge 1 commit intojd-opensource:mainfrom
DongheJin:feat/dtype
Open

feat: support parsing dtype field from config.#1118
DongheJin wants to merge 1 commit intojd-opensource:mainfrom
DongheJin:feat/dtype

Conversation

@DongheJin
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a normalize_config_torch_dtype function to standardize the handling of dtype and torch_dtype in model configuration files. Specifically, it ensures that if a configuration only provides dtype, it is also used for torch_dtype when loading model and quantization arguments. A new test case has been added to validate this behavior. The reviewer pointed out that the current implementation of normalize_config_torch_dtype involves an inefficient serialization/deserialization roundtrip for JsonReader creation, suggesting a more direct construction method for performance improvement.

}

JsonReader normalized_reader;
normalized_reader.parse_text(config.dump());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation of normalize_config_torch_dtype creates a new JsonReader by first serializing the nlohmann::json object to a string (config.dump()) and then deserializing it (normalized_reader.parse_text()). This roundtrip serialization and deserialization is inefficient and introduces unnecessary overhead. It would be more performant if the JsonReader class provided a constructor that directly accepts an nlohmann::json object, allowing for direct construction without the intermediate string conversion.

Copy link
Copy Markdown
Collaborator

@yq33victor yq33victor left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants