Conversation
This PR saves torch artifacts to be used later in evaluation or quantization. TICO-DCO-1.0-Signed-off-by: s.malakhov <s.malakhov@partner.samsung.com>
71fc131 to
750ab2d
Compare
| "--save_torch_artifacts_to_folder", | ||
| type=str, | ||
| default=None, | ||
| help="Save all layers to the folder specified", |
There was a problem hiding this comment.
Help message should be changed. Current one seems to be copied from other option.
| save_name = get_ptq_model_name(model, args) | ||
| save_path = pathlib.Path(args.save_torch_artifacts_to_folder, save_name) | ||
| print(f"Saving PTQ model to {save_path}") | ||
| torch.save(q_m, save_path) |
There was a problem hiding this comment.
Could you let me know what this saving is for?
Addionally, saving the entire module object with torch.save(q_m, ...) may be fragile across code changes and environments.
There was a problem hiding this comment.
@mhs4670go
please see #626, we can use it to evaluate saved model. gptq can be time consuming, as well as evaluation of quantized model. So if we have saved model, we can later reevaluate it using different set of benchmarks. Currently it's supposed to have single environment.
There was a problem hiding this comment.
Got it. How about adding a comment like this above the function call?
# Save quantized model for later re-evaluation (used by eval script to skip re-quantization)
torch.save(q_m, save_path)| help="Save all layers to the folder specified", | ||
| ) | ||
| parser.add_argument( | ||
| "--save_torch_artifacts_to_folder", |
There was a problem hiding this comment.
save_torch_artifacts_to_folder feels too broad and a bit misleading for what this option actually does.
Currently, this single flag is used to save multiple outputs (e.g., GPTQ sensitivities and a PTQ model checkpoint), which have different purposes and lifecycles.
Also, this is inconsistent with the existing saving options (save_circle_to_folder, save_layers_to_folder), which are all explicit about what they save. In contrast, this new option is abstract and mixes multiple artifact types under one flag.
It would be better to use a more specific name (e.g., save_quant_artifacts_to_folder) so that the behavior is more explicit, predictable, and consistent with the existing CLI design.
There was a problem hiding this comment.
I’m also a bit concerned about the overall save interface as it seems to be growing option by option.
At the moment, the save-related flags do not follow a single clear abstraction: some are named by format (circle), some by granularity (layers), and this new one by a very broad implementation-oriented term (torch artifacts). As more save targets get added, this may become harder to understand and maintain from the CLI side.
It may be worth considering a more structured interface, for example:
- one output directory option, and
- a separate argument that explicitly lists which artifacts to save
This would likely scale better than continuing to add one save flag per artifact/output type.
One of examples would be like below.
--output_dir ./outputs
--save sensitivity,ptq_checkpoint
# OR
--save model_circle sensitivity ptq_checkpointIf you don't have enough time for refactoring, you can just add a comment as TODO.
There was a problem hiding this comment.
Ahh. Ok. Let it be one folder with multiple options. I'll rework this PR.
This PR saves torch artifacts to be used later in evaluation or quantization.
sample output for `Maykeye/TinyLLama-v0`
TICO-DCO-1.0-Signed-off-by: s.malakhov s.malakhov@partner.samsung.com